Can we drop __must_check from driver_for_each_device()?

Can we drop __must_check from driver_for_each_device()?

[Paul Bolle]

Greg,

0) Summary: I think __must_check can be dropped from
driver_for_each_device(). Do you agree?

1) Commit 4a7fb6363f ("add __must_check to device management code")
added __must_check to driver_for_each_device(), seven years ago. But of
the seventeen current users of that function just one actually seems to
care about its return value:

- dead code:
  drivers/mtd/onenand/omap2.c:omap2_onenand_rephase()

- callback always returns zero:
  drivers/infiniband/hw/qib/qib_init.c:qib_notify_dca()
    (see all three possible values of f_notify_dca())
  drivers/media/pci/cx25821/cx25821-alsa.c:cx25821_audio_fini()
  drivers/media/pci/cx25821/cx25821-alsa.c:cx25821_alsa_init()
  drivers/net/can/usb/peak_usb/pcan_usb_core.c:peak_usb_exit()
  drivers/net/ethernet/intel/igb/igb_main.c:igb_notify_dca()
  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:ixgbe_notify_dca()
  drivers/net/ethernet/myricom/myri10ge/myri10ge.c:myri10ge_notify_dca()

- return value of callback ignored:
  drivers/iommu/omap-iommu.c:omap_foreach_iommu_device()
    (when called by iommu_debugfs_exit())
  drivers/isdn/hardware/mISDN/hfcpci.c:hfcpci_softirq()
  drivers/media/pci/cx18/cx18-alsa-main.c:cx18_alsa_exit()
  drivers/media/pci/ivtv/ivtv-alsa-main.c:ivtv_alsa_exit()
  drivers/media/pci/ivtv/ivtvfb.c:ivtvfb_init()
  drivers/media/pci/ivtv/ivtvfb.c:ivtvfb_cleanup()
  drivers/media/platform/s5p-tv/mixer_video.c:find_and_register_subdev()
  drivers/net/wireless/brcm80211/brcmfmac/usb.c:brcmf_usb_exit()

- return value actually used:
  drivers/iommu/omap-iommu.c:omap_foreach_iommu_device()
    (when called by iommu_debug_init())

2) Please note that if the callback always returns zero,
driver_for_each_device() can still return -EINVAL, but only if it was
provided a NULL "drv" (a struct device_driver). It sure seems odd to do
so. Can that actually happen?

3) So to me it looks the __must_check attribute of
driver_for_each_device() can be dropped. Do you agree?


Paul Bolle

Re: Can we drop __must_check from driver_for_each_device()?

[Greg Kroah-Hartman]

On Wed, Jul 31, 2013 at 11:35:13PM +0200, Paul Bolle wrote:
> Greg,
> 
> 0) Summary: I think __must_check can be dropped from
> driver_for_each_device(). Do you agree?

No.

> 1) Commit 4a7fb6363f ("add __must_check to device management code")
> added __must_check to driver_for_each_device(), seven years ago. But of
> the seventeen current users of that function just one actually seems to
> care about its return value:
> 
> - dead code:
>   drivers/mtd/onenand/omap2.c:omap2_onenand_rephase()
> 
> - callback always returns zero:
>   drivers/infiniband/hw/qib/qib_init.c:qib_notify_dca()
>     (see all three possible values of f_notify_dca())
>   drivers/media/pci/cx25821/cx25821-alsa.c:cx25821_audio_fini()
>   drivers/media/pci/cx25821/cx25821-alsa.c:cx25821_alsa_init()
>   drivers/net/can/usb/peak_usb/pcan_usb_core.c:peak_usb_exit()
>   drivers/net/ethernet/intel/igb/igb_main.c:igb_notify_dca()
>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:ixgbe_notify_dca()
>   drivers/net/ethernet/myricom/myri10ge/myri10ge.c:myri10ge_notify_dca()
> 
> - return value of callback ignored:
>   drivers/iommu/omap-iommu.c:omap_foreach_iommu_device()
>     (when called by iommu_debugfs_exit())
>   drivers/isdn/hardware/mISDN/hfcpci.c:hfcpci_softirq()
>   drivers/media/pci/cx18/cx18-alsa-main.c:cx18_alsa_exit()
>   drivers/media/pci/ivtv/ivtv-alsa-main.c:ivtv_alsa_exit()
>   drivers/media/pci/ivtv/ivtvfb.c:ivtvfb_init()
>   drivers/media/pci/ivtv/ivtvfb.c:ivtvfb_cleanup()
>   drivers/media/platform/s5p-tv/mixer_video.c:find_and_register_subdev()
>   drivers/net/wireless/brcm80211/brcmfmac/usb.c:brcmf_usb_exit()
> 
> - return value actually used:
>   drivers/iommu/omap-iommu.c:omap_foreach_iommu_device()
>     (when called by iommu_debug_init())
> 
> 2) Please note that if the callback always returns zero,
> driver_for_each_device() can still return -EINVAL, but only if it was
> provided a NULL "drv" (a struct device_driver). It sure seems odd to do
> so. Can that actually happen?

Possibly.

> 3) So to me it looks the __must_check attribute of
> driver_for_each_device() can be dropped. Do you agree?

Nope, it should be making people think about the return value of the
function.  If they use it or not might be a problem, but I would argue
that those call-sites must be fixed, as you point out above.

Is this somehow causing a problem that removing the marking would solve
for you?

thanks,

greg k-h

Re: Can we drop __must_check from driver_for_each_device()?

[Paul Bolle]

On Fri, 2013-08-02 at 08:31 +0800, Greg Kroah-Hartman wrote:
> On Wed, Jul 31, 2013 at 11:35:13PM +0200, Paul Bolle wrote:
> > 2) Please note that if the callback always returns zero,
> > driver_for_each_device() can still return -EINVAL, but only if it was
> > provided a NULL "drv" (a struct device_driver). It sure seems odd to do
> > so. Can that actually happen?
> 
> Possibly.

So driver_for_each_device() really should be NULL "drv" safe. But
wouldn't it therefor be better to make sure the callback functions do
not return -EINVAL themselves, so -EINVAL will always only indicate the
NULL "drv" case?

> > 3) So to me it looks the __must_check attribute of
> > driver_for_each_device() can be dropped. Do you agree?
> 
> Nope, it should be making people think about the return value of the
> function.  If they use it or not might be a problem, but I would argue
> that those call-sites must be fixed, as you point out above.

I see. I guess I should try to submit patches that do just that.

> Is this somehow causing a problem that removing the marking would solve
> for you?

The, rather trivial, issue I'd like to fix is this (long standing)
warning:
    drivers/isdn/hardware/mISDN/hfcpci.c:2298:2: warning: \
    ignoring return value of ‘driver_for_each_device’, \
    declared with attribute warn_unused_result [-Wunused-result]

I've submitted a patch to silence that warning about a year ago (see
https://lkml.org/lkml/2012/9/21/138 ). Dave Miller was pretty clear that
that approach wouldn't do. (I've added Dave to the CC, just because I
mentioned him here.)

So, since this warning is still there, I'm looking for another way to
get rid of it.

Thanks!


Paul Bolle

Re: Can we drop __must_check from driver_for_each_device()?

[Greg Kroah-Hartman]

On Tue, Aug 06, 2013 at 10:31:25PM +0200, Paul Bolle wrote:
> On Fri, 2013-08-02 at 08:31 +0800, Greg Kroah-Hartman wrote:
> > On Wed, Jul 31, 2013 at 11:35:13PM +0200, Paul Bolle wrote:
> > > 2) Please note that if the callback always returns zero,
> > > driver_for_each_device() can still return -EINVAL, but only if it was
> > > provided a NULL "drv" (a struct device_driver). It sure seems odd to do
> > > so. Can that actually happen?
> > 
> > Possibly.
> 
> So driver_for_each_device() really should be NULL "drv" safe.

Probably not, now that I think about it some more.  I don't see how that
could ever really happen, do you?

> But wouldn't it therefor be better to make sure the callback functions
> do not return -EINVAL themselves, so -EINVAL will always only indicate
> the NULL "drv" case?

I doubt it's a real need at all.

> > > 3) So to me it looks the __must_check attribute of
> > > driver_for_each_device() can be dropped. Do you agree?
> > 
> > Nope, it should be making people think about the return value of the
> > function.  If they use it or not might be a problem, but I would argue
> > that those call-sites must be fixed, as you point out above.
> 
> I see. I guess I should try to submit patches that do just that.
> 
> > Is this somehow causing a problem that removing the marking would solve
> > for you?
> 
> The, rather trivial, issue I'd like to fix is this (long standing)
> warning:
>     drivers/isdn/hardware/mISDN/hfcpci.c:2298:2: warning: \
>     ignoring return value of ‘driver_for_each_device’, \
>     declared with attribute warn_unused_result [-Wunused-result]
>
> I've submitted a patch to silence that warning about a year ago (see
> https://lkml.org/lkml/2012/9/21/138 ). Dave Miller was pretty clear that
> that approach wouldn't do. (I've added Dave to the CC, just because I
> mentioned him here.)

I agree with David, that patch is pointless.

> So, since this warning is still there, I'm looking for another way to
> get rid of it.

Fix it properly would be good to do, don't you think?

thanks,

greg k-h

Re: Can we drop __must_check from driver_for_each_device()?

[Paul Bolle]

On Wed, 2013-08-07 at 14:51 +0900, Greg Kroah-Hartman wrote:
> On Tue, Aug 06, 2013 at 10:31:25PM +0200, Paul Bolle wrote:
> > On Fri, 2013-08-02 at 08:31 +0800, Greg Kroah-Hartman wrote:
> > > On Wed, Jul 31, 2013 at 11:35:13PM +0200, Paul Bolle wrote:
> > > > 2) Please note that if the callback always returns zero,
> > > > driver_for_each_device() can still return -EINVAL, but only if it was
> > > > provided a NULL "drv" (a struct device_driver). It sure seems odd to do
> > > > so. Can that actually happen?
> > > 
> > > Possibly.
> > 
> > So driver_for_each_device() really should be NULL "drv" safe.
> 
> Probably not, now that I think about it some more.  I don't see how that
> could ever really happen, do you?

No, I don't.

See there are two groups of user of driver_for_each_device():
1) those that call it on the device_driver member of their
platform_driver, pci_driver, or usb_driver. If that device_driver member
turns out to be NULL we've got bigger problems, I guess;
2) those that call driver_find() to get a pointer to a device_driver.
These users can trivially check for NULL before calling
driver_for_each_device().

[Side note: the comment above driver_find() reads in part:
    This routine provides no locking to prevent the driver it returns
    from being unregistered or unloaded while the caller is using it.
    The caller is responsible for preventing this.

None of these callers of driver_find() seem to be troubled by this.
Their use of the pointer to a device_driver could be racey.]

Anyhow, I think the NULL "dev" check can be dropped.

> > But wouldn't it therefor be better to make sure the callback functions
> > do not return -EINVAL themselves, so -EINVAL will always only indicate
> > the NULL "drv" case?
> 
> I doubt it's a real need at all.

This is, of course, why I raised this issue. Because if we drop the NULL
"drv" check, we have a function that will in most cases always return
zero, because the callbacks it uses mostly return zero. So I think that
if the NULL "drv" check could be dropped, we might as well drop
__must_check here. The few cases were the callback might return non-zero
are already handled correctly.

> > So, since this warning is still there, I'm looking for another way to
> > get rid of it.
> 
> Fix it properly would be good to do, don't you think?

It's hard to disagree with that!

Thanks,


Paul Bolle