Re: [PATCH 1/3] driver core: add probe_err log helper

2018-10-17 Thread Joe Perches
On Wed, 2018-10-17 at 12:29 +0100, Russell King - ARM Linux wrote:
> On Tue, Oct 16, 2018 at 04:55:00PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 16, 2018 at 3:55 PM Andrzej Hajda  wrote:
> > > On 16.10.2018 13:29, Andrzej Hajda wrote:
> > > > On 16.10.2018 13:01, Andy Shevchenko wrote:
> > > > > On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda  
> > > > > wrote:
> > > > > > During probe every time driver gets resource it should usually 
> > > > > > check for error
> > > > > > printk some message if it is not -EPROBE_DEFER and return the 
> > > > > > error. This
> > > > > > pattern is simple but requires adding few lines after any resource 
> > > > > > acquisition
> > > > > > code, as a result it is often omited or implemented only partially.
> > > > > > probe_err helps to replace such code seqences with simple call, so 
> > > > > > code:
> > > > > > if (err != -EPROBE_DEFER)
> > > > > > dev_err(dev, ...);
> > > > > > return err;
> > > > > > becomes:
> > > > > > return probe_err(dev, err, ...);
> > > > > > +   va_start(args, fmt);
> > > > > > +
> > > > > > +   vaf.fmt = fmt;
> > > > > > +   vaf.va = 
> > > > > > +
> > > > > > +   __dev_printk(KERN_ERR, dev, );
> > > > > It would be nice to print an error code as well, wouldn't it?
> > > > Hmm, on probe fail error is printed anyway (with exception of
> > > > EPROBE_DEFER, ENODEV and ENXIO):
> > > > "probe of %s failed with error %d\n"
> > > > On the other side currently some drivers prints the error code anyway
> > > > via dev_err or similar, so I guess during conversion to probe_err it
> > > > should be removed then.
> > > > 
> > > > If we add error code to probe_err is it OK to report it this way?
> > > > dev_err(dev, "%V, %d\n", , err);
> > > 
> > > Ups, I forgot that message passed to probe_err will contain already
> > > newline character.
> > 
> > You may consider not to pass it.
> 
> It's normal to pass the '\n', so by doing this, we create the situation
> where this function becomes the exception to the norm.  That's not a
> good idea - we will see people forget that appending '\n' should not
> be done for this particular function.
> 
> While we could add a checkpatch rule, that's hassle (extra rework).

It would not be a simple checkpatch rule with high confidence
because of pr_cont uses that may not be in the patch context.

> In I think the message would be much better formatted if we did:
> 
>   dev_err(dev, "error %d: %V", err, );

s/%V/%pV/




Re: [PATCH 1/3] driver core: add probe_err log helper

2018-10-17 Thread Joe Perches
On Wed, 2018-10-17 at 12:29 +0100, Russell King - ARM Linux wrote:
> On Tue, Oct 16, 2018 at 04:55:00PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 16, 2018 at 3:55 PM Andrzej Hajda  wrote:
> > > On 16.10.2018 13:29, Andrzej Hajda wrote:
> > > > On 16.10.2018 13:01, Andy Shevchenko wrote:
> > > > > On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda  
> > > > > wrote:
> > > > > > During probe every time driver gets resource it should usually 
> > > > > > check for error
> > > > > > printk some message if it is not -EPROBE_DEFER and return the 
> > > > > > error. This
> > > > > > pattern is simple but requires adding few lines after any resource 
> > > > > > acquisition
> > > > > > code, as a result it is often omited or implemented only partially.
> > > > > > probe_err helps to replace such code seqences with simple call, so 
> > > > > > code:
> > > > > > if (err != -EPROBE_DEFER)
> > > > > > dev_err(dev, ...);
> > > > > > return err;
> > > > > > becomes:
> > > > > > return probe_err(dev, err, ...);
> > > > > > +   va_start(args, fmt);
> > > > > > +
> > > > > > +   vaf.fmt = fmt;
> > > > > > +   vaf.va = 
> > > > > > +
> > > > > > +   __dev_printk(KERN_ERR, dev, );
> > > > > It would be nice to print an error code as well, wouldn't it?
> > > > Hmm, on probe fail error is printed anyway (with exception of
> > > > EPROBE_DEFER, ENODEV and ENXIO):
> > > > "probe of %s failed with error %d\n"
> > > > On the other side currently some drivers prints the error code anyway
> > > > via dev_err or similar, so I guess during conversion to probe_err it
> > > > should be removed then.
> > > > 
> > > > If we add error code to probe_err is it OK to report it this way?
> > > > dev_err(dev, "%V, %d\n", , err);
> > > 
> > > Ups, I forgot that message passed to probe_err will contain already
> > > newline character.
> > 
> > You may consider not to pass it.
> 
> It's normal to pass the '\n', so by doing this, we create the situation
> where this function becomes the exception to the norm.  That's not a
> good idea - we will see people forget that appending '\n' should not
> be done for this particular function.
> 
> While we could add a checkpatch rule, that's hassle (extra rework).

It would not be a simple checkpatch rule with high confidence
because of pr_cont uses that may not be in the patch context.

> In I think the message would be much better formatted if we did:
> 
>   dev_err(dev, "error %d: %V", err, );

s/%V/%pV/




Re: [PATCH 1/3] driver core: add probe_err log helper

2018-10-17 Thread Andy Shevchenko
On Wed, Oct 17, 2018 at 2:29 PM Russell King - ARM Linux
 wrote:
>
> On Tue, Oct 16, 2018 at 04:55:00PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 16, 2018 at 3:55 PM Andrzej Hajda  wrote:
> > > On 16.10.2018 13:29, Andrzej Hajda wrote:
> > > > On 16.10.2018 13:01, Andy Shevchenko wrote:
> > > >> On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda  
> > > >> wrote:
> > > >>> During probe every time driver gets resource it should usually check 
> > > >>> for error
> > > >>> printk some message if it is not -EPROBE_DEFER and return the error. 
> > > >>> This
> > > >>> pattern is simple but requires adding few lines after any resource 
> > > >>> acquisition
> > > >>> code, as a result it is often omited or implemented only partially.
> > > >>> probe_err helps to replace such code seqences with simple call, so 
> > > >>> code:
> > > >>> if (err != -EPROBE_DEFER)
> > > >>> dev_err(dev, ...);
> > > >>> return err;
> > > >>> becomes:
> > > >>> return probe_err(dev, err, ...);
> >
> > > >>> +   va_start(args, fmt);
> > > >>> +
> > > >>> +   vaf.fmt = fmt;
> > > >>> +   vaf.va = 
> > > >>> +
> > > >>> +   __dev_printk(KERN_ERR, dev, );
> >
> > > >> It would be nice to print an error code as well, wouldn't it?
> > > > Hmm, on probe fail error is printed anyway (with exception of
> > > > EPROBE_DEFER, ENODEV and ENXIO):
> > > > "probe of %s failed with error %d\n"
> > > > On the other side currently some drivers prints the error code anyway
> > > > via dev_err or similar, so I guess during conversion to probe_err it
> > > > should be removed then.
> > > >
> > > > If we add error code to probe_err is it OK to report it this way?
> > > > dev_err(dev, "%V, %d\n", , err);
> > >
> > > Ups, I forgot that message passed to probe_err will contain already
> > > newline character.
> >
> > You may consider not to pass it.
>
> It's normal to pass the '\n', so by doing this, we create the situation
> where this function becomes the exception to the norm.  That's not a
> good idea - we will see people forget that appending '\n' should not
> be done for this particular function.
>
> While we could add a checkpatch rule, that's hassle (extra rework).  In
> any case, I think the message would be much better formatted if we did:
>
> dev_err(dev, "error %d: %V", err, );
>
> which means we end up with (eg):
>
> error -5: request_irq failed for irq 9
>
> rather than:
>
> request_irq failed for irq 9, -5
>
> which is more confusing.

As I said earlier, I'm fine to either variant and I see your point
here, so, I tend to agree to this variant.

P.S. Andrzej, in any case my Rb tag, which I just gave, stays.

-- 
With Best Regards,
Andy Shevchenko


Re: [PATCH 1/3] driver core: add probe_err log helper

2018-10-17 Thread Andy Shevchenko
On Wed, Oct 17, 2018 at 2:29 PM Russell King - ARM Linux
 wrote:
>
> On Tue, Oct 16, 2018 at 04:55:00PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 16, 2018 at 3:55 PM Andrzej Hajda  wrote:
> > > On 16.10.2018 13:29, Andrzej Hajda wrote:
> > > > On 16.10.2018 13:01, Andy Shevchenko wrote:
> > > >> On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda  
> > > >> wrote:
> > > >>> During probe every time driver gets resource it should usually check 
> > > >>> for error
> > > >>> printk some message if it is not -EPROBE_DEFER and return the error. 
> > > >>> This
> > > >>> pattern is simple but requires adding few lines after any resource 
> > > >>> acquisition
> > > >>> code, as a result it is often omited or implemented only partially.
> > > >>> probe_err helps to replace such code seqences with simple call, so 
> > > >>> code:
> > > >>> if (err != -EPROBE_DEFER)
> > > >>> dev_err(dev, ...);
> > > >>> return err;
> > > >>> becomes:
> > > >>> return probe_err(dev, err, ...);
> >
> > > >>> +   va_start(args, fmt);
> > > >>> +
> > > >>> +   vaf.fmt = fmt;
> > > >>> +   vaf.va = 
> > > >>> +
> > > >>> +   __dev_printk(KERN_ERR, dev, );
> >
> > > >> It would be nice to print an error code as well, wouldn't it?
> > > > Hmm, on probe fail error is printed anyway (with exception of
> > > > EPROBE_DEFER, ENODEV and ENXIO):
> > > > "probe of %s failed with error %d\n"
> > > > On the other side currently some drivers prints the error code anyway
> > > > via dev_err or similar, so I guess during conversion to probe_err it
> > > > should be removed then.
> > > >
> > > > If we add error code to probe_err is it OK to report it this way?
> > > > dev_err(dev, "%V, %d\n", , err);
> > >
> > > Ups, I forgot that message passed to probe_err will contain already
> > > newline character.
> >
> > You may consider not to pass it.
>
> It's normal to pass the '\n', so by doing this, we create the situation
> where this function becomes the exception to the norm.  That's not a
> good idea - we will see people forget that appending '\n' should not
> be done for this particular function.
>
> While we could add a checkpatch rule, that's hassle (extra rework).  In
> any case, I think the message would be much better formatted if we did:
>
> dev_err(dev, "error %d: %V", err, );
>
> which means we end up with (eg):
>
> error -5: request_irq failed for irq 9
>
> rather than:
>
> request_irq failed for irq 9, -5
>
> which is more confusing.

As I said earlier, I'm fine to either variant and I see your point
here, so, I tend to agree to this variant.

P.S. Andrzej, in any case my Rb tag, which I just gave, stays.

-- 
With Best Regards,
Andy Shevchenko


Re: [PATCH 1/3] driver core: add probe_err log helper

2018-10-17 Thread Russell King - ARM Linux
On Tue, Oct 16, 2018 at 04:55:00PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 16, 2018 at 3:55 PM Andrzej Hajda  wrote:
> > On 16.10.2018 13:29, Andrzej Hajda wrote:
> > > On 16.10.2018 13:01, Andy Shevchenko wrote:
> > >> On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda  
> > >> wrote:
> > >>> During probe every time driver gets resource it should usually check 
> > >>> for error
> > >>> printk some message if it is not -EPROBE_DEFER and return the error. 
> > >>> This
> > >>> pattern is simple but requires adding few lines after any resource 
> > >>> acquisition
> > >>> code, as a result it is often omited or implemented only partially.
> > >>> probe_err helps to replace such code seqences with simple call, so code:
> > >>> if (err != -EPROBE_DEFER)
> > >>> dev_err(dev, ...);
> > >>> return err;
> > >>> becomes:
> > >>> return probe_err(dev, err, ...);
> 
> > >>> +   va_start(args, fmt);
> > >>> +
> > >>> +   vaf.fmt = fmt;
> > >>> +   vaf.va = 
> > >>> +
> > >>> +   __dev_printk(KERN_ERR, dev, );
> 
> > >> It would be nice to print an error code as well, wouldn't it?
> > > Hmm, on probe fail error is printed anyway (with exception of
> > > EPROBE_DEFER, ENODEV and ENXIO):
> > > "probe of %s failed with error %d\n"
> > > On the other side currently some drivers prints the error code anyway
> > > via dev_err or similar, so I guess during conversion to probe_err it
> > > should be removed then.
> > >
> > > If we add error code to probe_err is it OK to report it this way?
> > > dev_err(dev, "%V, %d\n", , err);
> >
> > Ups, I forgot that message passed to probe_err will contain already
> > newline character.
> 
> You may consider not to pass it.

It's normal to pass the '\n', so by doing this, we create the situation
where this function becomes the exception to the norm.  That's not a
good idea - we will see people forget that appending '\n' should not
be done for this particular function.

While we could add a checkpatch rule, that's hassle (extra rework).  In
any case, I think the message would be much better formatted if we did:

dev_err(dev, "error %d: %V", err, );

which means we end up with (eg):

error -5: request_irq failed for irq 9

rather than:

request_irq failed for irq 9, -5

which is more confusing.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up


Re: [PATCH 1/3] driver core: add probe_err log helper

2018-10-17 Thread Russell King - ARM Linux
On Tue, Oct 16, 2018 at 04:55:00PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 16, 2018 at 3:55 PM Andrzej Hajda  wrote:
> > On 16.10.2018 13:29, Andrzej Hajda wrote:
> > > On 16.10.2018 13:01, Andy Shevchenko wrote:
> > >> On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda  
> > >> wrote:
> > >>> During probe every time driver gets resource it should usually check 
> > >>> for error
> > >>> printk some message if it is not -EPROBE_DEFER and return the error. 
> > >>> This
> > >>> pattern is simple but requires adding few lines after any resource 
> > >>> acquisition
> > >>> code, as a result it is often omited or implemented only partially.
> > >>> probe_err helps to replace such code seqences with simple call, so code:
> > >>> if (err != -EPROBE_DEFER)
> > >>> dev_err(dev, ...);
> > >>> return err;
> > >>> becomes:
> > >>> return probe_err(dev, err, ...);
> 
> > >>> +   va_start(args, fmt);
> > >>> +
> > >>> +   vaf.fmt = fmt;
> > >>> +   vaf.va = 
> > >>> +
> > >>> +   __dev_printk(KERN_ERR, dev, );
> 
> > >> It would be nice to print an error code as well, wouldn't it?
> > > Hmm, on probe fail error is printed anyway (with exception of
> > > EPROBE_DEFER, ENODEV and ENXIO):
> > > "probe of %s failed with error %d\n"
> > > On the other side currently some drivers prints the error code anyway
> > > via dev_err or similar, so I guess during conversion to probe_err it
> > > should be removed then.
> > >
> > > If we add error code to probe_err is it OK to report it this way?
> > > dev_err(dev, "%V, %d\n", , err);
> >
> > Ups, I forgot that message passed to probe_err will contain already
> > newline character.
> 
> You may consider not to pass it.

It's normal to pass the '\n', so by doing this, we create the situation
where this function becomes the exception to the norm.  That's not a
good idea - we will see people forget that appending '\n' should not
be done for this particular function.

While we could add a checkpatch rule, that's hassle (extra rework).  In
any case, I think the message would be much better formatted if we did:

dev_err(dev, "error %d: %V", err, );

which means we end up with (eg):

error -5: request_irq failed for irq 9

rather than:

request_irq failed for irq 9, -5

which is more confusing.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up


Re: [PATCH 1/3] driver core: add probe_err log helper

2018-10-16 Thread Andy Shevchenko
On Tue, Oct 16, 2018 at 3:55 PM Andrzej Hajda  wrote:
> On 16.10.2018 13:29, Andrzej Hajda wrote:
> > On 16.10.2018 13:01, Andy Shevchenko wrote:
> >> On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda  wrote:
> >>> During probe every time driver gets resource it should usually check for 
> >>> error
> >>> printk some message if it is not -EPROBE_DEFER and return the error. This
> >>> pattern is simple but requires adding few lines after any resource 
> >>> acquisition
> >>> code, as a result it is often omited or implemented only partially.
> >>> probe_err helps to replace such code seqences with simple call, so code:
> >>> if (err != -EPROBE_DEFER)
> >>> dev_err(dev, ...);
> >>> return err;
> >>> becomes:
> >>> return probe_err(dev, err, ...);

> >>> +   va_start(args, fmt);
> >>> +
> >>> +   vaf.fmt = fmt;
> >>> +   vaf.va = 
> >>> +
> >>> +   __dev_printk(KERN_ERR, dev, );

> >> It would be nice to print an error code as well, wouldn't it?
> > Hmm, on probe fail error is printed anyway (with exception of
> > EPROBE_DEFER, ENODEV and ENXIO):
> > "probe of %s failed with error %d\n"
> > On the other side currently some drivers prints the error code anyway
> > via dev_err or similar, so I guess during conversion to probe_err it
> > should be removed then.
> >
> > If we add error code to probe_err is it OK to report it this way?
> > dev_err(dev, "%V, %d\n", , err);
>
> Ups, I forgot that message passed to probe_err will contain already
> newline character.

You may consider not to pass it.

> So the err must be before message passed to probe_err, for example:
> dev_err(dev, "err=%d: %V\n", err, );

> Is it OK?

For me would work either (no \n in the message, or err preceding the message).

-- 
With Best Regards,
Andy Shevchenko


Re: [PATCH 1/3] driver core: add probe_err log helper

2018-10-16 Thread Andy Shevchenko
On Tue, Oct 16, 2018 at 3:55 PM Andrzej Hajda  wrote:
> On 16.10.2018 13:29, Andrzej Hajda wrote:
> > On 16.10.2018 13:01, Andy Shevchenko wrote:
> >> On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda  wrote:
> >>> During probe every time driver gets resource it should usually check for 
> >>> error
> >>> printk some message if it is not -EPROBE_DEFER and return the error. This
> >>> pattern is simple but requires adding few lines after any resource 
> >>> acquisition
> >>> code, as a result it is often omited or implemented only partially.
> >>> probe_err helps to replace such code seqences with simple call, so code:
> >>> if (err != -EPROBE_DEFER)
> >>> dev_err(dev, ...);
> >>> return err;
> >>> becomes:
> >>> return probe_err(dev, err, ...);

> >>> +   va_start(args, fmt);
> >>> +
> >>> +   vaf.fmt = fmt;
> >>> +   vaf.va = 
> >>> +
> >>> +   __dev_printk(KERN_ERR, dev, );

> >> It would be nice to print an error code as well, wouldn't it?
> > Hmm, on probe fail error is printed anyway (with exception of
> > EPROBE_DEFER, ENODEV and ENXIO):
> > "probe of %s failed with error %d\n"
> > On the other side currently some drivers prints the error code anyway
> > via dev_err or similar, so I guess during conversion to probe_err it
> > should be removed then.
> >
> > If we add error code to probe_err is it OK to report it this way?
> > dev_err(dev, "%V, %d\n", , err);
>
> Ups, I forgot that message passed to probe_err will contain already
> newline character.

You may consider not to pass it.

> So the err must be before message passed to probe_err, for example:
> dev_err(dev, "err=%d: %V\n", err, );

> Is it OK?

For me would work either (no \n in the message, or err preceding the message).

-- 
With Best Regards,
Andy Shevchenko


Re: [PATCH 1/3] driver core: add probe_err log helper

2018-10-16 Thread Andrzej Hajda
On 16.10.2018 13:29, Andrzej Hajda wrote:
> On 16.10.2018 13:01, Andy Shevchenko wrote:
>> On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda  wrote:
>>> During probe every time driver gets resource it should usually check for 
>>> error
>>> printk some message if it is not -EPROBE_DEFER and return the error. This
>>> pattern is simple but requires adding few lines after any resource 
>>> acquisition
>>> code, as a result it is often omited or implemented only partially.
>>> probe_err helps to replace such code seqences with simple call, so code:
>>> if (err != -EPROBE_DEFER)
>>> dev_err(dev, ...);
>>> return err;
>>> becomes:
>>> return probe_err(dev, err, ...);
>>>
>>> Signed-off-by: Andrzej Hajda 
>>> ---
>>>  drivers/base/core.c| 37 +
>>>  include/linux/device.h |  2 ++
>>>  2 files changed, 39 insertions(+)
>>>
>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>>> index 04bbcd779e11..23fabefb217a 100644
>>> --- a/drivers/base/core.c
>>> +++ b/drivers/base/core.c
>>> @@ -3067,6 +3067,43 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>>>
>>>  #endif
>>>
>>> +/**
>>> + * probe_err - probe error check and log helper
>>> + * @dev: the pointer to the struct device
>>> + * @err: error value to test
>>> + * @fmt: printf-style format string
>>> + * @...: arguments as specified in the format string
>>> + *
>>> + * This helper implements common pattern present in probe functions for 
>>> error
>>> + * checking: print message if the error is not -EPROBE_DEFER and propagate 
>>> it.
>>> + * It replaces code sequence:
>>> + * if (err != -EPROBE_DEFER)
>>> + * dev_err(dev, ...);
>>> + * return err;
>>> + * with
>>> + * return probe_err(dev, err, ...);
>>> + *
>>> + * Returns @err.
>>> + *
>>> + */
>>> +int probe_err(const struct device *dev, int err, const char *fmt, ...)
>>> +{
>>> +   struct va_format vaf;
>>> +   va_list args;
>>> +
>>> +   if (err != -EPROBE_DEFER) {
>> Why not
>>
>> if (err == ...)
>>  return err;
>>
>> ...
>> return err;
>>
>> ?
>>
>> Better to read, better to maintain. No?
> Yes, anyway next patch will re-factor it anyway.
>
>>> +   va_start(args, fmt);
>>> +
>>> +   vaf.fmt = fmt;
>>> +   vaf.va = 
>>> +
>>> +   __dev_printk(KERN_ERR, dev, );
>> It would be nice to print an error code as well, wouldn't it?
> Hmm, on probe fail error is printed anyway (with exception of
> EPROBE_DEFER, ENODEV and ENXIO):
>     "probe of %s failed with error %d\n"
> On the other side currently some drivers prints the error code anyway
> via dev_err or similar, so I guess during conversion to probe_err it
> should be removed then.
>
> If we add error code to probe_err is it OK to report it this way?
>     dev_err(dev, "%V, %d\n", , err);

Ups, I forgot that message passed to probe_err will contain already
newline character.
So the err must be before message passed to probe_err, for example:
    dev_err(dev, "err=%d: %V\n", err, );
Is it OK? Or better leave this part of the patch as is?

Regards
Andrzej

>
> Regards
> Andrzej
>
>>> +   va_end(args);
>>> +   }
>>> +
>>> +   return err;
>>> +}
>>> +
>>>  static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>>>  {
>>> return fwnode && !IS_ERR(fwnode->secondary);
>>> diff --git a/include/linux/device.h b/include/linux/device.h
>>> index 90224e75ade4..06c2c797d132 100644
>>> --- a/include/linux/device.h
>>> +++ b/include/linux/device.h
>>> @@ -1577,6 +1577,8 @@ do {  
>>> \
>>> WARN_ONCE(condition, "%s %s: " format, \
>>> dev_driver_string(dev), dev_name(dev), ## arg)
>>>
>>> +int probe_err(const struct device *dev, int err, const char *fmt, ...);
>>> +
>>>  /* Create alias, so I can be autoloaded. */
>>>  #define MODULE_ALIAS_CHARDEV(major,minor) \
>>> MODULE_ALIAS("char-major-" __stringify(major) "-" 
>>> __stringify(minor))
>>> --
>>> 2.18.0
>>>



Re: [PATCH 1/3] driver core: add probe_err log helper

2018-10-16 Thread Andrzej Hajda
On 16.10.2018 13:29, Andrzej Hajda wrote:
> On 16.10.2018 13:01, Andy Shevchenko wrote:
>> On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda  wrote:
>>> During probe every time driver gets resource it should usually check for 
>>> error
>>> printk some message if it is not -EPROBE_DEFER and return the error. This
>>> pattern is simple but requires adding few lines after any resource 
>>> acquisition
>>> code, as a result it is often omited or implemented only partially.
>>> probe_err helps to replace such code seqences with simple call, so code:
>>> if (err != -EPROBE_DEFER)
>>> dev_err(dev, ...);
>>> return err;
>>> becomes:
>>> return probe_err(dev, err, ...);
>>>
>>> Signed-off-by: Andrzej Hajda 
>>> ---
>>>  drivers/base/core.c| 37 +
>>>  include/linux/device.h |  2 ++
>>>  2 files changed, 39 insertions(+)
>>>
>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>>> index 04bbcd779e11..23fabefb217a 100644
>>> --- a/drivers/base/core.c
>>> +++ b/drivers/base/core.c
>>> @@ -3067,6 +3067,43 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>>>
>>>  #endif
>>>
>>> +/**
>>> + * probe_err - probe error check and log helper
>>> + * @dev: the pointer to the struct device
>>> + * @err: error value to test
>>> + * @fmt: printf-style format string
>>> + * @...: arguments as specified in the format string
>>> + *
>>> + * This helper implements common pattern present in probe functions for 
>>> error
>>> + * checking: print message if the error is not -EPROBE_DEFER and propagate 
>>> it.
>>> + * It replaces code sequence:
>>> + * if (err != -EPROBE_DEFER)
>>> + * dev_err(dev, ...);
>>> + * return err;
>>> + * with
>>> + * return probe_err(dev, err, ...);
>>> + *
>>> + * Returns @err.
>>> + *
>>> + */
>>> +int probe_err(const struct device *dev, int err, const char *fmt, ...)
>>> +{
>>> +   struct va_format vaf;
>>> +   va_list args;
>>> +
>>> +   if (err != -EPROBE_DEFER) {
>> Why not
>>
>> if (err == ...)
>>  return err;
>>
>> ...
>> return err;
>>
>> ?
>>
>> Better to read, better to maintain. No?
> Yes, anyway next patch will re-factor it anyway.
>
>>> +   va_start(args, fmt);
>>> +
>>> +   vaf.fmt = fmt;
>>> +   vaf.va = 
>>> +
>>> +   __dev_printk(KERN_ERR, dev, );
>> It would be nice to print an error code as well, wouldn't it?
> Hmm, on probe fail error is printed anyway (with exception of
> EPROBE_DEFER, ENODEV and ENXIO):
>     "probe of %s failed with error %d\n"
> On the other side currently some drivers prints the error code anyway
> via dev_err or similar, so I guess during conversion to probe_err it
> should be removed then.
>
> If we add error code to probe_err is it OK to report it this way?
>     dev_err(dev, "%V, %d\n", , err);

Ups, I forgot that message passed to probe_err will contain already
newline character.
So the err must be before message passed to probe_err, for example:
    dev_err(dev, "err=%d: %V\n", err, );
Is it OK? Or better leave this part of the patch as is?

Regards
Andrzej

>
> Regards
> Andrzej
>
>>> +   va_end(args);
>>> +   }
>>> +
>>> +   return err;
>>> +}
>>> +
>>>  static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>>>  {
>>> return fwnode && !IS_ERR(fwnode->secondary);
>>> diff --git a/include/linux/device.h b/include/linux/device.h
>>> index 90224e75ade4..06c2c797d132 100644
>>> --- a/include/linux/device.h
>>> +++ b/include/linux/device.h
>>> @@ -1577,6 +1577,8 @@ do {  
>>> \
>>> WARN_ONCE(condition, "%s %s: " format, \
>>> dev_driver_string(dev), dev_name(dev), ## arg)
>>>
>>> +int probe_err(const struct device *dev, int err, const char *fmt, ...);
>>> +
>>>  /* Create alias, so I can be autoloaded. */
>>>  #define MODULE_ALIAS_CHARDEV(major,minor) \
>>> MODULE_ALIAS("char-major-" __stringify(major) "-" 
>>> __stringify(minor))
>>> --
>>> 2.18.0
>>>



Re: [PATCH 1/3] driver core: add probe_err log helper

2018-10-16 Thread Andrzej Hajda
On 16.10.2018 13:01, Andy Shevchenko wrote:
> On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda  wrote:
>> During probe every time driver gets resource it should usually check for 
>> error
>> printk some message if it is not -EPROBE_DEFER and return the error. This
>> pattern is simple but requires adding few lines after any resource 
>> acquisition
>> code, as a result it is often omited or implemented only partially.
>> probe_err helps to replace such code seqences with simple call, so code:
>> if (err != -EPROBE_DEFER)
>> dev_err(dev, ...);
>> return err;
>> becomes:
>> return probe_err(dev, err, ...);
>>
>> Signed-off-by: Andrzej Hajda 
>> ---
>>  drivers/base/core.c| 37 +
>>  include/linux/device.h |  2 ++
>>  2 files changed, 39 insertions(+)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 04bbcd779e11..23fabefb217a 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -3067,6 +3067,43 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>>
>>  #endif
>>
>> +/**
>> + * probe_err - probe error check and log helper
>> + * @dev: the pointer to the struct device
>> + * @err: error value to test
>> + * @fmt: printf-style format string
>> + * @...: arguments as specified in the format string
>> + *
>> + * This helper implements common pattern present in probe functions for 
>> error
>> + * checking: print message if the error is not -EPROBE_DEFER and propagate 
>> it.
>> + * It replaces code sequence:
>> + * if (err != -EPROBE_DEFER)
>> + * dev_err(dev, ...);
>> + * return err;
>> + * with
>> + * return probe_err(dev, err, ...);
>> + *
>> + * Returns @err.
>> + *
>> + */
>> +int probe_err(const struct device *dev, int err, const char *fmt, ...)
>> +{
>> +   struct va_format vaf;
>> +   va_list args;
>> +
>> +   if (err != -EPROBE_DEFER) {
> Why not
>
> if (err == ...)
>  return err;
>
> ...
> return err;
>
> ?
>
> Better to read, better to maintain. No?

Yes, anyway next patch will re-factor it anyway.

>
>> +   va_start(args, fmt);
>> +
>> +   vaf.fmt = fmt;
>> +   vaf.va = 
>> +
>> +   __dev_printk(KERN_ERR, dev, );
> It would be nice to print an error code as well, wouldn't it?

Hmm, on probe fail error is printed anyway (with exception of
EPROBE_DEFER, ENODEV and ENXIO):
    "probe of %s failed with error %d\n"
On the other side currently some drivers prints the error code anyway
via dev_err or similar, so I guess during conversion to probe_err it
should be removed then.

If we add error code to probe_err is it OK to report it this way?
    dev_err(dev, "%V, %d\n", , err);

Regards
Andrzej

>
>> +   va_end(args);
>> +   }
>> +
>> +   return err;
>> +}
>> +
>>  static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>>  {
>> return fwnode && !IS_ERR(fwnode->secondary);
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 90224e75ade4..06c2c797d132 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -1577,6 +1577,8 @@ do {   
>>\
>> WARN_ONCE(condition, "%s %s: " format, \
>> dev_driver_string(dev), dev_name(dev), ## arg)
>>
>> +int probe_err(const struct device *dev, int err, const char *fmt, ...);
>> +
>>  /* Create alias, so I can be autoloaded. */
>>  #define MODULE_ALIAS_CHARDEV(major,minor) \
>> MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
>> --
>> 2.18.0
>>
>



Re: [PATCH 1/3] driver core: add probe_err log helper

2018-10-16 Thread Andrzej Hajda
On 16.10.2018 13:01, Andy Shevchenko wrote:
> On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda  wrote:
>> During probe every time driver gets resource it should usually check for 
>> error
>> printk some message if it is not -EPROBE_DEFER and return the error. This
>> pattern is simple but requires adding few lines after any resource 
>> acquisition
>> code, as a result it is often omited or implemented only partially.
>> probe_err helps to replace such code seqences with simple call, so code:
>> if (err != -EPROBE_DEFER)
>> dev_err(dev, ...);
>> return err;
>> becomes:
>> return probe_err(dev, err, ...);
>>
>> Signed-off-by: Andrzej Hajda 
>> ---
>>  drivers/base/core.c| 37 +
>>  include/linux/device.h |  2 ++
>>  2 files changed, 39 insertions(+)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 04bbcd779e11..23fabefb217a 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -3067,6 +3067,43 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>>
>>  #endif
>>
>> +/**
>> + * probe_err - probe error check and log helper
>> + * @dev: the pointer to the struct device
>> + * @err: error value to test
>> + * @fmt: printf-style format string
>> + * @...: arguments as specified in the format string
>> + *
>> + * This helper implements common pattern present in probe functions for 
>> error
>> + * checking: print message if the error is not -EPROBE_DEFER and propagate 
>> it.
>> + * It replaces code sequence:
>> + * if (err != -EPROBE_DEFER)
>> + * dev_err(dev, ...);
>> + * return err;
>> + * with
>> + * return probe_err(dev, err, ...);
>> + *
>> + * Returns @err.
>> + *
>> + */
>> +int probe_err(const struct device *dev, int err, const char *fmt, ...)
>> +{
>> +   struct va_format vaf;
>> +   va_list args;
>> +
>> +   if (err != -EPROBE_DEFER) {
> Why not
>
> if (err == ...)
>  return err;
>
> ...
> return err;
>
> ?
>
> Better to read, better to maintain. No?

Yes, anyway next patch will re-factor it anyway.

>
>> +   va_start(args, fmt);
>> +
>> +   vaf.fmt = fmt;
>> +   vaf.va = 
>> +
>> +   __dev_printk(KERN_ERR, dev, );
> It would be nice to print an error code as well, wouldn't it?

Hmm, on probe fail error is printed anyway (with exception of
EPROBE_DEFER, ENODEV and ENXIO):
    "probe of %s failed with error %d\n"
On the other side currently some drivers prints the error code anyway
via dev_err or similar, so I guess during conversion to probe_err it
should be removed then.

If we add error code to probe_err is it OK to report it this way?
    dev_err(dev, "%V, %d\n", , err);

Regards
Andrzej

>
>> +   va_end(args);
>> +   }
>> +
>> +   return err;
>> +}
>> +
>>  static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>>  {
>> return fwnode && !IS_ERR(fwnode->secondary);
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 90224e75ade4..06c2c797d132 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -1577,6 +1577,8 @@ do {   
>>\
>> WARN_ONCE(condition, "%s %s: " format, \
>> dev_driver_string(dev), dev_name(dev), ## arg)
>>
>> +int probe_err(const struct device *dev, int err, const char *fmt, ...);
>> +
>>  /* Create alias, so I can be autoloaded. */
>>  #define MODULE_ALIAS_CHARDEV(major,minor) \
>> MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
>> --
>> 2.18.0
>>
>



Re: [PATCH 1/3] driver core: add probe_err log helper

2018-10-16 Thread Greg Kroah-Hartman
On Tue, Oct 16, 2018 at 11:27:15AM +0100, Mark Brown wrote:
> On Tue, Oct 16, 2018 at 09:22:42AM +0200, Andrzej Hajda wrote:
> 
> > +int probe_err(const struct device *dev, int err, const char *fmt, ...)
> > +{
> > +   struct va_format vaf;
> > +   va_list args;
> 
> ...
> 
> > +   return err;
> > +}
> > +
> 
> This will need an EXPORT_SYMBOL for modules won't it?

EXPORT_SYMBOL_GPL() to be specific, as it is dealing with the driver
core.

thanks,

greg k-h


Re: [PATCH 1/3] driver core: add probe_err log helper

2018-10-16 Thread Greg Kroah-Hartman
On Tue, Oct 16, 2018 at 11:27:15AM +0100, Mark Brown wrote:
> On Tue, Oct 16, 2018 at 09:22:42AM +0200, Andrzej Hajda wrote:
> 
> > +int probe_err(const struct device *dev, int err, const char *fmt, ...)
> > +{
> > +   struct va_format vaf;
> > +   va_list args;
> 
> ...
> 
> > +   return err;
> > +}
> > +
> 
> This will need an EXPORT_SYMBOL for modules won't it?

EXPORT_SYMBOL_GPL() to be specific, as it is dealing with the driver
core.

thanks,

greg k-h


Re: [PATCH 1/3] driver core: add probe_err log helper

2018-10-16 Thread Andy Shevchenko
On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda  wrote:
>
> During probe every time driver gets resource it should usually check for error
> printk some message if it is not -EPROBE_DEFER and return the error. This
> pattern is simple but requires adding few lines after any resource acquisition
> code, as a result it is often omited or implemented only partially.
> probe_err helps to replace such code seqences with simple call, so code:
> if (err != -EPROBE_DEFER)
> dev_err(dev, ...);
> return err;
> becomes:
> return probe_err(dev, err, ...);
>
> Signed-off-by: Andrzej Hajda 
> ---
>  drivers/base/core.c| 37 +
>  include/linux/device.h |  2 ++
>  2 files changed, 39 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 04bbcd779e11..23fabefb217a 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3067,6 +3067,43 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>
>  #endif
>
> +/**
> + * probe_err - probe error check and log helper
> + * @dev: the pointer to the struct device
> + * @err: error value to test
> + * @fmt: printf-style format string
> + * @...: arguments as specified in the format string
> + *
> + * This helper implements common pattern present in probe functions for error
> + * checking: print message if the error is not -EPROBE_DEFER and propagate 
> it.
> + * It replaces code sequence:
> + * if (err != -EPROBE_DEFER)
> + * dev_err(dev, ...);
> + * return err;
> + * with
> + * return probe_err(dev, err, ...);
> + *
> + * Returns @err.
> + *
> + */
> +int probe_err(const struct device *dev, int err, const char *fmt, ...)
> +{
> +   struct va_format vaf;
> +   va_list args;
> +

> +   if (err != -EPROBE_DEFER) {

Why not

if (err == ...)
 return err;

...
return err;

?

Better to read, better to maintain. No?

> +   va_start(args, fmt);
> +
> +   vaf.fmt = fmt;
> +   vaf.va = 
> +

> +   __dev_printk(KERN_ERR, dev, );

It would be nice to print an error code as well, wouldn't it?

> +   va_end(args);
> +   }
> +
> +   return err;
> +}
> +
>  static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>  {
> return fwnode && !IS_ERR(fwnode->secondary);
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 90224e75ade4..06c2c797d132 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1577,6 +1577,8 @@ do {
>   \
> WARN_ONCE(condition, "%s %s: " format, \
> dev_driver_string(dev), dev_name(dev), ## arg)
>
> +int probe_err(const struct device *dev, int err, const char *fmt, ...);
> +
>  /* Create alias, so I can be autoloaded. */
>  #define MODULE_ALIAS_CHARDEV(major,minor) \
> MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
> --
> 2.18.0
>


-- 
With Best Regards,
Andy Shevchenko


Re: [PATCH 1/3] driver core: add probe_err log helper

2018-10-16 Thread Andy Shevchenko
On Tue, Oct 16, 2018 at 10:22 AM Andrzej Hajda  wrote:
>
> During probe every time driver gets resource it should usually check for error
> printk some message if it is not -EPROBE_DEFER and return the error. This
> pattern is simple but requires adding few lines after any resource acquisition
> code, as a result it is often omited or implemented only partially.
> probe_err helps to replace such code seqences with simple call, so code:
> if (err != -EPROBE_DEFER)
> dev_err(dev, ...);
> return err;
> becomes:
> return probe_err(dev, err, ...);
>
> Signed-off-by: Andrzej Hajda 
> ---
>  drivers/base/core.c| 37 +
>  include/linux/device.h |  2 ++
>  2 files changed, 39 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 04bbcd779e11..23fabefb217a 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3067,6 +3067,43 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>
>  #endif
>
> +/**
> + * probe_err - probe error check and log helper
> + * @dev: the pointer to the struct device
> + * @err: error value to test
> + * @fmt: printf-style format string
> + * @...: arguments as specified in the format string
> + *
> + * This helper implements common pattern present in probe functions for error
> + * checking: print message if the error is not -EPROBE_DEFER and propagate 
> it.
> + * It replaces code sequence:
> + * if (err != -EPROBE_DEFER)
> + * dev_err(dev, ...);
> + * return err;
> + * with
> + * return probe_err(dev, err, ...);
> + *
> + * Returns @err.
> + *
> + */
> +int probe_err(const struct device *dev, int err, const char *fmt, ...)
> +{
> +   struct va_format vaf;
> +   va_list args;
> +

> +   if (err != -EPROBE_DEFER) {

Why not

if (err == ...)
 return err;

...
return err;

?

Better to read, better to maintain. No?

> +   va_start(args, fmt);
> +
> +   vaf.fmt = fmt;
> +   vaf.va = 
> +

> +   __dev_printk(KERN_ERR, dev, );

It would be nice to print an error code as well, wouldn't it?

> +   va_end(args);
> +   }
> +
> +   return err;
> +}
> +
>  static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>  {
> return fwnode && !IS_ERR(fwnode->secondary);
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 90224e75ade4..06c2c797d132 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1577,6 +1577,8 @@ do {
>   \
> WARN_ONCE(condition, "%s %s: " format, \
> dev_driver_string(dev), dev_name(dev), ## arg)
>
> +int probe_err(const struct device *dev, int err, const char *fmt, ...);
> +
>  /* Create alias, so I can be autoloaded. */
>  #define MODULE_ALIAS_CHARDEV(major,minor) \
> MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
> --
> 2.18.0
>


-- 
With Best Regards,
Andy Shevchenko


Re: [PATCH 1/3] driver core: add probe_err log helper

2018-10-16 Thread Mark Brown
On Tue, Oct 16, 2018 at 09:22:42AM +0200, Andrzej Hajda wrote:

> +int probe_err(const struct device *dev, int err, const char *fmt, ...)
> +{
> + struct va_format vaf;
> + va_list args;

...

> + return err;
> +}
> +

This will need an EXPORT_SYMBOL for modules won't it?


signature.asc
Description: PGP signature


Re: [PATCH 1/3] driver core: add probe_err log helper

2018-10-16 Thread Mark Brown
On Tue, Oct 16, 2018 at 09:22:42AM +0200, Andrzej Hajda wrote:

> +int probe_err(const struct device *dev, int err, const char *fmt, ...)
> +{
> + struct va_format vaf;
> + va_list args;

...

> + return err;
> +}
> +

This will need an EXPORT_SYMBOL for modules won't it?


signature.asc
Description: PGP signature


Re: [PATCH 1/3] driver core: add probe_err log helper

2018-10-16 Thread Javier Martinez Canillas
Hello Andrzej,

On 10/16/2018 09:22 AM, Andrzej Hajda wrote:
> During probe every time driver gets resource it should usually check for error
> printk some message if it is not -EPROBE_DEFER and return the error. This
> pattern is simple but requires adding few lines after any resource acquisition
> code, as a result it is often omited or implemented only partially.
> probe_err helps to replace such code seqences with simple call, so code:
>   if (err != -EPROBE_DEFER)
>   dev_err(dev, ...);
>   return err;
> becomes:
>   return probe_err(dev, err, ...);
> 
> Signed-off-by: Andrzej Hajda 
> ---

Reviewed-by: Javier Martinez Canillas 

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat


Re: [PATCH 1/3] driver core: add probe_err log helper

2018-10-16 Thread Javier Martinez Canillas
Hello Andrzej,

On 10/16/2018 09:22 AM, Andrzej Hajda wrote:
> During probe every time driver gets resource it should usually check for error
> printk some message if it is not -EPROBE_DEFER and return the error. This
> pattern is simple but requires adding few lines after any resource acquisition
> code, as a result it is often omited or implemented only partially.
> probe_err helps to replace such code seqences with simple call, so code:
>   if (err != -EPROBE_DEFER)
>   dev_err(dev, ...);
>   return err;
> becomes:
>   return probe_err(dev, err, ...);
> 
> Signed-off-by: Andrzej Hajda 
> ---

Reviewed-by: Javier Martinez Canillas 

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat


[PATCH 1/3] driver core: add probe_err log helper

2018-10-16 Thread Andrzej Hajda
During probe every time driver gets resource it should usually check for error
printk some message if it is not -EPROBE_DEFER and return the error. This
pattern is simple but requires adding few lines after any resource acquisition
code, as a result it is often omited or implemented only partially.
probe_err helps to replace such code seqences with simple call, so code:
if (err != -EPROBE_DEFER)
dev_err(dev, ...);
return err;
becomes:
return probe_err(dev, err, ...);

Signed-off-by: Andrzej Hajda 
---
 drivers/base/core.c| 37 +
 include/linux/device.h |  2 ++
 2 files changed, 39 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 04bbcd779e11..23fabefb217a 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3067,6 +3067,43 @@ define_dev_printk_level(_dev_info, KERN_INFO);
 
 #endif
 
+/**
+ * probe_err - probe error check and log helper
+ * @dev: the pointer to the struct device
+ * @err: error value to test
+ * @fmt: printf-style format string
+ * @...: arguments as specified in the format string
+ *
+ * This helper implements common pattern present in probe functions for error
+ * checking: print message if the error is not -EPROBE_DEFER and propagate it.
+ * It replaces code sequence:
+ * if (err != -EPROBE_DEFER)
+ * dev_err(dev, ...);
+ * return err;
+ * with
+ * return probe_err(dev, err, ...);
+ *
+ * Returns @err.
+ *
+ */
+int probe_err(const struct device *dev, int err, const char *fmt, ...)
+{
+   struct va_format vaf;
+   va_list args;
+
+   if (err != -EPROBE_DEFER) {
+   va_start(args, fmt);
+
+   vaf.fmt = fmt;
+   vaf.va = 
+
+   __dev_printk(KERN_ERR, dev, );
+   va_end(args);
+   }
+
+   return err;
+}
+
 static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
 {
return fwnode && !IS_ERR(fwnode->secondary);
diff --git a/include/linux/device.h b/include/linux/device.h
index 90224e75ade4..06c2c797d132 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1577,6 +1577,8 @@ do {  
\
WARN_ONCE(condition, "%s %s: " format, \
dev_driver_string(dev), dev_name(dev), ## arg)
 
+int probe_err(const struct device *dev, int err, const char *fmt, ...);
+
 /* Create alias, so I can be autoloaded. */
 #define MODULE_ALIAS_CHARDEV(major,minor) \
MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
-- 
2.18.0



[PATCH 1/3] driver core: add probe_err log helper

2018-10-16 Thread Andrzej Hajda
During probe every time driver gets resource it should usually check for error
printk some message if it is not -EPROBE_DEFER and return the error. This
pattern is simple but requires adding few lines after any resource acquisition
code, as a result it is often omited or implemented only partially.
probe_err helps to replace such code seqences with simple call, so code:
if (err != -EPROBE_DEFER)
dev_err(dev, ...);
return err;
becomes:
return probe_err(dev, err, ...);

Signed-off-by: Andrzej Hajda 
---
 drivers/base/core.c| 37 +
 include/linux/device.h |  2 ++
 2 files changed, 39 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 04bbcd779e11..23fabefb217a 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3067,6 +3067,43 @@ define_dev_printk_level(_dev_info, KERN_INFO);
 
 #endif
 
+/**
+ * probe_err - probe error check and log helper
+ * @dev: the pointer to the struct device
+ * @err: error value to test
+ * @fmt: printf-style format string
+ * @...: arguments as specified in the format string
+ *
+ * This helper implements common pattern present in probe functions for error
+ * checking: print message if the error is not -EPROBE_DEFER and propagate it.
+ * It replaces code sequence:
+ * if (err != -EPROBE_DEFER)
+ * dev_err(dev, ...);
+ * return err;
+ * with
+ * return probe_err(dev, err, ...);
+ *
+ * Returns @err.
+ *
+ */
+int probe_err(const struct device *dev, int err, const char *fmt, ...)
+{
+   struct va_format vaf;
+   va_list args;
+
+   if (err != -EPROBE_DEFER) {
+   va_start(args, fmt);
+
+   vaf.fmt = fmt;
+   vaf.va = 
+
+   __dev_printk(KERN_ERR, dev, );
+   va_end(args);
+   }
+
+   return err;
+}
+
 static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
 {
return fwnode && !IS_ERR(fwnode->secondary);
diff --git a/include/linux/device.h b/include/linux/device.h
index 90224e75ade4..06c2c797d132 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1577,6 +1577,8 @@ do {  
\
WARN_ONCE(condition, "%s %s: " format, \
dev_driver_string(dev), dev_name(dev), ## arg)
 
+int probe_err(const struct device *dev, int err, const char *fmt, ...);
+
 /* Create alias, so I can be autoloaded. */
 #define MODULE_ALIAS_CHARDEV(major,minor) \
MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
-- 
2.18.0