[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <e3f8fe62-71ae-4fde-9274-62fcbcf7dcb9@quicinc.com>
Date: Tue, 13 Aug 2024 19:01:05 +0800
From: quic_zijuhu <quic_zijuhu@...cinc.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Zijun Hu
<zijun_hu@...oud.com>
CC: Dave Ertman <david.m.ertman@...el.com>, Ira Weiny <ira.weiny@...el.com>,
"Rafael J. Wysocki" <rafael@...nel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] drivers/base: Introduce device_match_t for device
finding APIs
On 8/13/2024 5:52 PM, Greg Kroah-Hartman wrote:
> On Sun, Aug 11, 2024 at 10:15:16AM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@...cinc.com>
>>
>> There are several drivers/base APIs for finding a specific device, and
>> they currently use the following good type for the @match parameter:
>> int (*match)(struct device *dev, const void *data)
>>
>> Since these operations do not modify the caller-provided @*data, this
>> type is worthy of a dedicated typedef:
>> typedef int (*device_match_t)(struct device *dev, const void *data)
>>
>> Advantages of using device_match_t:
>> - Shorter API declarations and definitions
>> - Prevent further APIs from using a bad type for @match
>>
>> So introduce device_match_t and apply it to the existing
>> (bus|class|driver|auxiliary)_find_device() APIs.
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@...cinc.com>
>> ---
>> Changes in v2:
>> - Git rebase over the following commit:
>> bfa54a793ba7 ("driver core: bus: Fix double free in driver API bus_register()")
>> - Link to v1: https://lore.kernel.org/r/20240802-dev_match_api-v1-1-51e16d3bf031@quicinc.com
>> ---
>> drivers/base/auxiliary.c | 2 +-
>> drivers/base/bus.c | 2 +-
>> drivers/base/class.c | 3 +--
>> drivers/base/driver.c | 2 +-
>> include/linux/auxiliary_bus.h | 2 +-
>> include/linux/device/bus.h | 5 +++--
>> include/linux/device/class.h | 2 +-
>> include/linux/device/driver.h | 2 +-
>> 8 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c
>> index 54b92839e05c..7823888af4f6 100644
>> --- a/drivers/base/auxiliary.c
>> +++ b/drivers/base/auxiliary.c
>> @@ -352,7 +352,7 @@ EXPORT_SYMBOL_GPL(__auxiliary_device_add);
>> */
>> struct auxiliary_device *auxiliary_find_device(struct device *start,
>> const void *data,
>> - int (*match)(struct device *dev, const void *data))
>> + device_match_t match)
>> {
>> struct device *dev;
>>
>> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
>> index abf090ace833..657c93c38b0d 100644
>> --- a/drivers/base/bus.c
>> +++ b/drivers/base/bus.c
>> @@ -391,7 +391,7 @@ EXPORT_SYMBOL_GPL(bus_for_each_dev);
>> */
>> struct device *bus_find_device(const struct bus_type *bus,
>> struct device *start, const void *data,
>> - int (*match)(struct device *dev, const void *data))
>> + device_match_t match)
>> {
>> struct subsys_private *sp = bus_to_subsys(bus);
>> struct klist_iter i;
>> diff --git a/drivers/base/class.c b/drivers/base/class.c
>> index 7b38fdf8e1d7..ae22fa992c04 100644
>> --- a/drivers/base/class.c
>> +++ b/drivers/base/class.c
>> @@ -433,8 +433,7 @@ EXPORT_SYMBOL_GPL(class_for_each_device);
>> * code. There's no locking restriction.
>> */
>> struct device *class_find_device(const struct class *class, const struct device *start,
>> - const void *data,
>> - int (*match)(struct device *, const void *))
>> + const void *data, device_match_t match)
>> {
>> struct subsys_private *sp = class_to_subsys(class);
>> struct class_dev_iter iter;
>> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
>> index 88c6fd1f1992..b4eb5b89c4ee 100644
>> --- a/drivers/base/driver.c
>> +++ b/drivers/base/driver.c
>> @@ -150,7 +150,7 @@ EXPORT_SYMBOL_GPL(driver_for_each_device);
>> */
>> struct device *driver_find_device(const struct device_driver *drv,
>> struct device *start, const void *data,
>> - int (*match)(struct device *dev, const void *data))
>> + device_match_t match)
>> {
>> struct klist_iter i;
>> struct device *dev;
>> diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
>> index 662b8ae54b6a..31762324bcc9 100644
>> --- a/include/linux/auxiliary_bus.h
>> +++ b/include/linux/auxiliary_bus.h
>> @@ -271,6 +271,6 @@ void auxiliary_driver_unregister(struct auxiliary_driver *auxdrv);
>>
>> struct auxiliary_device *auxiliary_find_device(struct device *start,
>> const void *data,
>> - int (*match)(struct device *dev, const void *data));
>> + device_match_t match);
>>
>> #endif /* _AUXILIARY_BUS_H_ */
>> diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
>> index 807831d6bf0f..970de5ee5562 100644
>> --- a/include/linux/device/bus.h
>> +++ b/include/linux/device/bus.h
>> @@ -126,6 +126,8 @@ struct bus_attribute {
>> int __must_check bus_create_file(const struct bus_type *bus, struct bus_attribute *attr);
>> void bus_remove_file(const struct bus_type *bus, struct bus_attribute *attr);
>>
>> +/* Matching function type for drivers/base APIs to find a specific device */
>> +typedef int (*device_match_t)(struct device *dev, const void *data);
>> /* Generic device matching functions that all busses can use to match with */
>
> Extra new-line after the typedef please.
>
okay, let me correct it with v2.
> And shouldn't this be in device.h instead?
>
there is the good location to place with below reasons:
1) place this typedef and various match functions with the type
together, these functions immediately follow the type.
2) this type are also used by bus.c/class.c/driver.c which include bus.h
but don't include device.h
3) device.h have included bus.h
> thanks,
>
> greg k-h
Powered by blists - more mailing lists