| Message ID | 127dcc7f60d15a1cc9007c9e5b06a1aa2b170e19.1726237379.git.pnewman@connecttech.com |
|---|---|
| State | New |
| Headers | show |
| Series | serial: 8250_exar: Replace custom EEPROM code with eeprom_93cx6 | expand |
On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote: > From: Parker Newman <pnewman@connecttech.com> > > Replace printk(KERN_ERR ...) with pr_err() to improve readability. > > Fixes checkpatch warning: > > WARNING: Prefer [subsystem eg: netdev]_err([subsystem]dev, ... then > dev_err(dev, ... then pr_err(... to printk(KERN_ERR ... > + printk(KERN_ERR "%s: timeout\n", __func__); First of all, you probably want pr_fmt() to be defined. Second, I would replace the entire while loop by the read_poll_timeout() macro from iopoll.h (note to use "true" for sleep before check and drop that one from before the loop in the driver). Naturally the pr_err() change can be combined with read_poll_timeout(), but if you go with pr_fmt() perhaps it still makes sense to have separate changes. I dunno which one is better, up to you.
On Fri, 13 Sep 2024 20:54:05 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote: > > From: Parker Newman <pnewman@connecttech.com> > > > > Replace printk(KERN_ERR ...) with pr_err() to improve readability. > > > > Fixes checkpatch warning: > > > > WARNING: Prefer [subsystem eg: netdev]_err([subsystem]dev, ... then > > dev_err(dev, ... then pr_err(... to printk(KERN_ERR ... > > + printk(KERN_ERR "%s: timeout\n", __func__); > > First of all, you probably want pr_fmt() to be defined. Good point, I will add. > Second, I would replace the entire while loop by the read_poll_timeout() macro > from iopoll.h (note to use "true" for sleep before check and drop that one from > before the loop in the driver). Good idea. > Naturally the pr_err() change can be combined with read_poll_timeout(), but > if you go with pr_fmt() perhaps it still makes sense to have separate changes. > I dunno which one is better, up to you. > Sorry if I am miss-reading but do you mean the pr_err() and pr_fmt() can be combined and the read_poll_timeout() change should be made in a separate patch after? Or should I be adding the pr_fmt() define in its own patch, followed by the pr_err() and read_poll_timeout() in a patch? Thanks, Parker
On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote: > From: Parker Newman <pnewman@connecttech.com> > > Replace printk(KERN_ERR ...) with pr_err() to improve readability. > > Fixes checkpatch warning: > > WARNING: Prefer [subsystem eg: netdev]_err([subsystem]dev, ... then > dev_err(dev, ... then pr_err(... to printk(KERN_ERR ... > + printk(KERN_ERR "%s: timeout\n", __func__); > > Signed-off-by: Parker Newman <pnewman@connecttech.com> > --- > drivers/misc/eeprom/eeprom_93cx6.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/misc/eeprom/eeprom_93cx6.c b/drivers/misc/eeprom/eeprom_93cx6.c > index 64da22edefa4..755be9a4ffd4 100644 > --- a/drivers/misc/eeprom/eeprom_93cx6.c > +++ b/drivers/misc/eeprom/eeprom_93cx6.c > @@ -378,7 +378,7 @@ void eeprom_93cx6_write(struct eeprom_93cx6 *eeprom, u8 addr, u16 data) > usleep_range(1000, 2000); > > if (--timeout <= 0) { > - printk(KERN_ERR "%s: timeout\n", __func__); > + pr_err("%s: timeout\n", __func__); It's a device, please use dev_err(). thanks, greg k-h
On Sat, Sep 14, 2024 at 08:58:50PM +0200, Greg Kroah-Hartman wrote: > On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote: ... > > - printk(KERN_ERR "%s: timeout\n", __func__); > > + pr_err("%s: timeout\n", __func__); > > It's a device, please use dev_err(). The problem is that this library doesn't know about this fact. I.e. it would need a new member just for this message. Instead, maybe drop the message as we anyway get a unique enough error code?
On Mon, Sep 16, 2024 at 12:55:05PM +0300, Andy Shevchenko wrote: > On Sat, Sep 14, 2024 at 08:58:50PM +0200, Greg Kroah-Hartman wrote: > > On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote: > > ... > > > > - printk(KERN_ERR "%s: timeout\n", __func__); > > > + pr_err("%s: timeout\n", __func__); > > > > It's a device, please use dev_err(). > > The problem is that this library doesn't know about this fact. I.e. it would > need a new member just for this message. Instead, maybe drop the message as we > anyway get a unique enough error code? Fair enough, although adding real device pointers would be good to do in the future... thanks, greg k-h
On Mon, Sep 16, 2024 at 12:25:52PM +0200, Greg Kroah-Hartman wrote: > On Mon, Sep 16, 2024 at 12:55:05PM +0300, Andy Shevchenko wrote: > > On Sat, Sep 14, 2024 at 08:58:50PM +0200, Greg Kroah-Hartman wrote: > > > On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote: ... > > > > - printk(KERN_ERR "%s: timeout\n", __func__); > > > > + pr_err("%s: timeout\n", __func__); > > > > > > It's a device, please use dev_err(). > > > > The problem is that this library doesn't know about this fact. I.e. it would > > need a new member just for this message. Instead, maybe drop the message as we > > anyway get a unique enough error code? > > Fair enough, although adding real device pointers would be good to do in > the future... Let's then do it when it will be the real need? Because I don't think this message is _so_ important. I believe one of the upper layers (whichever calls this function) should propagate the error code up to the user space. If it's not the case _that_ has to be fixed. TL;DR: Let's remove the message for now.
On Mon, 16 Sep 2024 13:32:47 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Mon, Sep 16, 2024 at 12:25:52PM +0200, Greg Kroah-Hartman wrote: > > On Mon, Sep 16, 2024 at 12:55:05PM +0300, Andy Shevchenko wrote: > > > On Sat, Sep 14, 2024 at 08:58:50PM +0200, Greg Kroah-Hartman wrote: > > > > On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote: > > ... > > > > > > - printk(KERN_ERR "%s: timeout\n", __func__); > > > > > + pr_err("%s: timeout\n", __func__); > > > > > > > > It's a device, please use dev_err(). > > > > > > The problem is that this library doesn't know about this fact. I.e. it would > > > need a new member just for this message. Instead, maybe drop the message as we > > > anyway get a unique enough error code? > > > > Fair enough, although adding real device pointers would be good to do in > > the future... > > Let's then do it when it will be the real need? Because I don't think this > message is _so_ important. I believe one of the upper layers (whichever calls > this function) should propagate the error code up to the user space. If it's > not the case _that_ has to be fixed. > > TL;DR: Let's remove the message for now. > I can remove the message or leave it as is and drop this patch from the series. One could make the argument that any error indication it is better than none in this case.
On Mon, Sep 16, 2024 at 08:04:10AM -0400, Parker Newman wrote: > On Mon, 16 Sep 2024 13:32:47 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Sep 16, 2024 at 12:25:52PM +0200, Greg Kroah-Hartman wrote: > > > On Mon, Sep 16, 2024 at 12:55:05PM +0300, Andy Shevchenko wrote: > > > > On Sat, Sep 14, 2024 at 08:58:50PM +0200, Greg Kroah-Hartman wrote: > > > > > On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote: ... > > > > > > - printk(KERN_ERR "%s: timeout\n", __func__); > > > > > > + pr_err("%s: timeout\n", __func__); > > > > > > > > > > It's a device, please use dev_err(). > > > > > > > > The problem is that this library doesn't know about this fact. I.e. it would > > > > need a new member just for this message. Instead, maybe drop the message as we > > > > anyway get a unique enough error code? > > > > > > Fair enough, although adding real device pointers would be good to do in > > > the future... > > > > Let's then do it when it will be the real need? Because I don't think this > > message is _so_ important. I believe one of the upper layers (whichever calls > > this function) should propagate the error code up to the user space. If it's > > not the case _that_ has to be fixed. > > > > TL;DR: Let's remove the message for now. > > I can remove the message or leave it as is and drop this patch from the series. > One could make the argument that any error indication it is better than none > in this case. I think you can drop the message and make the patch to be last in the series, so it can be easily abandoned (in case that decision will be made) without throttling the rest. At the same time in the commit message explain that with move to read_poll_timeout() we drop the seems redundant message. I'm fine with that approach. But at the end of the day it's not that critical to the main purpose, i.e. cleaning up the Exar serial driver.
On Mon, 16 Sep 2024 15:18:15 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Mon, Sep 16, 2024 at 08:04:10AM -0400, Parker Newman wrote: > > On Mon, 16 Sep 2024 13:32:47 +0300 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > On Mon, Sep 16, 2024 at 12:25:52PM +0200, Greg Kroah-Hartman wrote: > > > > On Mon, Sep 16, 2024 at 12:55:05PM +0300, Andy Shevchenko wrote: > > > > > On Sat, Sep 14, 2024 at 08:58:50PM +0200, Greg Kroah-Hartman wrote: > > > > > > On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote: > > ... > > > > > > > > - printk(KERN_ERR "%s: timeout\n", __func__); > > > > > > > + pr_err("%s: timeout\n", __func__); > > > > > > > > > > > > It's a device, please use dev_err(). > > > > > > > > > > The problem is that this library doesn't know about this fact. I.e. it would > > > > > need a new member just for this message. Instead, maybe drop the message as we > > > > > anyway get a unique enough error code? > > > > > > > > Fair enough, although adding real device pointers would be good to do in > > > > the future... > > > > > > Let's then do it when it will be the real need? Because I don't think this > > > message is _so_ important. I believe one of the upper layers (whichever calls > > > this function) should propagate the error code up to the user space. If it's > > > not the case _that_ has to be fixed. > > > > > > TL;DR: Let's remove the message for now. > > > > I can remove the message or leave it as is and drop this patch from the series. > > One could make the argument that any error indication it is better than none > > in this case. > > I think you can drop the message and make the patch to be last in the series, > so it can be easily abandoned (in case that decision will be made) without > throttling the rest. At the same time in the commit message explain that with > move to read_poll_timeout() we drop the seems redundant message. I'm fine with > that approach. But at the end of the day it's not that critical to the main > purpose, i.e. cleaning up the Exar serial driver. > I don't think read_poll_timeout() will work directly because eeprom->register_read() does not return a value. I could add a "is write complete" wrapper function to work around that I guess. However, I think I will just drop this patch from the series as fixing it properly will be a big change and like you said its not critical to the main patch.
On Mon, Sep 16, 2024 at 11:20:56AM -0400, Parker Newman wrote: > On Mon, 16 Sep 2024 15:18:15 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Sep 16, 2024 at 08:04:10AM -0400, Parker Newman wrote: > > > On Mon, 16 Sep 2024 13:32:47 +0300 > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > On Mon, Sep 16, 2024 at 12:25:52PM +0200, Greg Kroah-Hartman wrote: > > > > > On Mon, Sep 16, 2024 at 12:55:05PM +0300, Andy Shevchenko wrote: > > > > > > On Sat, Sep 14, 2024 at 08:58:50PM +0200, Greg Kroah-Hartman wrote: > > > > > > > On Fri, Sep 13, 2024 at 10:55:40AM -0400, Parker Newman wrote: ... > > > > > > > > - printk(KERN_ERR "%s: timeout\n", __func__); > > > > > > > > + pr_err("%s: timeout\n", __func__); > > > > > > > > > > > > > > It's a device, please use dev_err(). > > > > > > > > > > > > The problem is that this library doesn't know about this fact. I.e. it would > > > > > > need a new member just for this message. Instead, maybe drop the message as we > > > > > > anyway get a unique enough error code? > > > > > > > > > > Fair enough, although adding real device pointers would be good to do in > > > > > the future... > > > > > > > > Let's then do it when it will be the real need? Because I don't think this > > > > message is _so_ important. I believe one of the upper layers (whichever calls > > > > this function) should propagate the error code up to the user space. If it's > > > > not the case _that_ has to be fixed. > > > > > > > > TL;DR: Let's remove the message for now. > > > > > > I can remove the message or leave it as is and drop this patch from the series. > > > One could make the argument that any error indication it is better than none > > > in this case. > > > > I think you can drop the message and make the patch to be last in the series, > > so it can be easily abandoned (in case that decision will be made) without > > throttling the rest. At the same time in the commit message explain that with > > move to read_poll_timeout() we drop the seems redundant message. I'm fine with > > that approach. But at the end of the day it's not that critical to the main > > purpose, i.e. cleaning up the Exar serial driver. > > I don't think read_poll_timeout() will work directly because eeprom->register_read() > does not return a value. I could add a "is write complete" wrapper function > to work around that I guess. However, I think I will just drop this patch from > the series as fixing it properly will be a big change and like you said its not > critical to the main patch. Sure, fine with me.
diff --git a/drivers/misc/eeprom/eeprom_93cx6.c b/drivers/misc/eeprom/eeprom_93cx6.c index 64da22edefa4..755be9a4ffd4 100644 --- a/drivers/misc/eeprom/eeprom_93cx6.c +++ b/drivers/misc/eeprom/eeprom_93cx6.c @@ -378,7 +378,7 @@ void eeprom_93cx6_write(struct eeprom_93cx6 *eeprom, u8 addr, u16 data) usleep_range(1000, 2000); if (--timeout <= 0) { - printk(KERN_ERR "%s: timeout\n", __func__); + pr_err("%s: timeout\n", __func__); break; } }