Re: [PATCH v1 1/2] ACPI / utils: Introduce acpi_dev_get_first_device() helper

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Thu, 2018-07-12 at 21:20 +0200, Hans de Goede wrote:
> Hi,
> 
> On 12-07-18 19:23, Andy Shevchenko wrote:
> > acpi_dev_present() and acpi_dev_get_first_match_name() are missing
> > put_device() call and thus keeping reference counting unbalanced.
> > 
> > In order to fix the issue introduce a new helper to convert existing
> > users
> > one-by-one to a better API.
> > 
> > 

> > +bool acpi_dev_present(const char *hid, const char *uid, s64 hrv)
> > +{
> > +	struct acpi_device *adev = acpi_dev_get_first_match(hid,
> > uid, hrv);
> > +
> > +	return !!adev;
> >   }
> >   EXPORT_SYMBOL(acpi_dev_present);
> 
> Why not just do:
> 
> bool acpi_dev_present(const char *hid, const char *uid, s64 hrv)
> {
> 	struct acpi_device *adev = acpi_dev_get_first_match(hid, uid,
> hrv);
> 
> 	if (!adev)
> 		return false;
> 
> 	put_device(&adev->dev);
> 	return true;
> }
> 
> And not deprecate this. This fixes the leak while keeping the
> API usage simple for users of this API. Having to do a put_device
> in all callers seems cumbersome if we can just do it here.

The new API has been dictated by the nature of bus_find_device().
Thus same rules applies.

However, in this very particular case (since we are a) just checking for
presense, and b) don't care if device will gone) it's okay like you
proposed.

So, would you agree on splitting this to several changes for better
granularity, i.e.

 - introduce new API
 - convert acpi_dev_present() to use it
 - fix the issue (squash with previous?)
 - ... do similar to acpi_dev_get_first_match_name() ...

?

Of course, we can fix acpi_dev_present() right now w/o new API, but it
feels to be half baked without fixing acpi_dev_get_first_match_name().

-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html