linux-kernel - Re: [PATCH 1/3] driver core: Mark impossible return values of bus

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5a1191d5-beaa-4bf6-9f68-4322cab96edf@icloud.com>
Date: Wed, 4 Sep 2024 23:45:31 +0800
From: Zijun Hu <zijun_hu@...oud.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, linux-kernel@...r.kernel.org,
 Zijun Hu <quic_zijuhu@...cinc.com>
Subject: Re: [PATCH 1/3] driver core: Mark impossible return values of
 bus_type's match() with unlikely()

On 2024/9/4 21:53, Greg Kroah-Hartman wrote:
> On Wed, Sep 04, 2024 at 08:56:42PM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@...cinc.com>
>>
>> Bus_type's match() should return bool type compatible integer 0 or 1
>> ideally since its main operations are lookup and comparison normally
>> actually, this rule is followed by ALL bus_types but @amba_bustype within
>> current v6.10 kernel tree, for @amba_bustype, ONLY extra -EPROBE_DEFER
>> may be returned, so mark those impossible or rare return values with
>> unlikely() to help readers understand device and driver binding logic.
> 
> unlikely() and likely() should ONLY be used when you can measure the
> performance impact.  And any "scan all devices in the bus" function is
> NOT performance critical at all.  So this is not the place for that,
> sorry.
> 

make sense
thank you for these explanation.

>> Signed-off-by: Zijun Hu <quic_zijuhu@...cinc.com>
>> ---
>>  drivers/base/dd.c          | 16 ++++++++++++----
>>  include/linux/device/bus.h |  9 ++++-----
>>  2 files changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index 9b745ba54de1..288e19c9854b 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -928,7 +928,11 @@ static int __device_attach_driver(struct device_driver *drv, void *_data)
>>  	if (ret == 0) {
>>  		/* no match */
>>  		return 0;
>> -	} else if (ret == -EPROBE_DEFER) {
>> +	} else if (unlikely(ret == -EPROBE_DEFER)) {
>> +		/*
>> +		 * Only match() of @amba_bustype may return this error
>> +		 * in current v6.10 tree, so also give unlikely() here.
> 
> version numbers in the kernel source mean nothing, and they age
> horribly. This is not going to work out at all.
> 
> let's fix up the one user that is doing this wrong and then we don't
> have to worry about it, right?  We have the source for everything, let's
> use it :)
> 
yes, you are right.

my original motivation is to see if it is possible change bus_type's
match() return value type to bool type based on below reasons:

(1)
it is not good time for bus_type's match() to operate device such as
I/O since device may be not ready to operate at that time.

(2)
match() is called without holding device lock firstly by driver_attach().

(3)
all bus_type's match() only do lookup and comparison and return bool
type but @amba_bustype which operate device and maybe return extra
-EPROBE_DEFER

> thanks,
> 
> greg k-h