linux-kernel - Re: Re: [RFC PATCH 3/4] i2c: core: treat EPROBE

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <15a449fa-d649-846a-e6f2-1540f9581846@microchip.com>
Date:   Mon, 3 Aug 2020 15:33:41 +0000
From:   <Codrin.Ciubotariu@...rochip.com>
To:     <wsa@...nel.org>
CC:     <linux-i2c@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <robh+dt@...nel.org>,
        <Ludovic.Desroches@...rochip.com>, <Nicolas.Ferre@...rochip.com>,
        <alexandre.belloni@...tlin.com>, <linux@...linux.org.uk>,
        <kamel.bouhara@...tlin.com>
Subject: Re: Re: [RFC PATCH 3/4] i2c: core: treat EPROBE_DEFER when acquiring
 SCL/SDA GPIOs

On 02.08.2020 20:05, Wolfram Sang wrote:
> On Fri, Jun 19, 2020 at 05:19:03PM +0300, Codrin Ciubotariu wrote:
>> Even if I2C bus GPIO recovery is optional, devm_gpiod_get() can return
>> -EPROBE_DEFER, so we should at least treat that. This ends up with
>> i2c_register_adapter() to be able to return -EPROBE_DEFER.
>>
>> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@...rochip.com>
>> ---
>>   drivers/i2c/i2c-core-base.c | 22 ++++++++++++++++------
>>   1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>> index 4ee29fec4e93..f8d9f2048ca8 100644
>> --- a/drivers/i2c/i2c-core-base.c
>> +++ b/drivers/i2c/i2c-core-base.c
>> @@ -368,15 +368,16 @@ static int i2c_gpio_init_recovery(struct i2c_adapter *adap)
>>   	return i2c_gpio_init_generic_recovery(adap);
>>   }
>>   
>> -static void i2c_init_recovery(struct i2c_adapter *adap)
>> +static int i2c_init_recovery(struct i2c_adapter *adap)
>>   {
>>   	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>>   	char *err_str;
>>   
>>   	if (!bri)
>> -		return;
>> +		return 0;
>>   
>> -	i2c_gpio_init_recovery(adap);
>> +	if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER)
>> +		return -EPROBE_DEFER;
>>   
>>   	if (!bri->recover_bus) {
>>   		err_str = "no recover_bus() found";
>> @@ -392,7 +393,7 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
>>   			if (gpiod_get_direction(bri->sda_gpiod) == 0)
>>   				bri->set_sda = set_sda_gpio_value;
>>   		}
>> -		return;
>> +		return 0;
> 
> This is correct but I think the code flow is/was confusing. Can you drop
> this 'return' and use 'else if' for the next code block? I think this is
> more readable.

Ok, it makes sense. Should I make a separate patch for this only?
One more question, should we keep:
if (!bri->set_sda && !bri->get_sda) {
	err_str = "either get_sda() or set_sda() needed";
	goto err;
}
?
Without {get/set}_sda we won't be able to generate stop commands and 
possibly check if the bus is free, but we can still generate the SCL 
clock pulses.

> 
>>   	}
>>   
>>   	if (bri->recover_bus == i2c_generic_scl_recovery) {
>> @@ -407,10 +408,12 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
>>   		}
>>   	}
>>   
>> -	return;
>> +	return 0;
>>    err:
>>   	dev_err(&adap->dev, "Not using recovery: %s\n", err_str);
>>   	adap->bus_recovery_info = NULL;
>> +
>> +	return 0;
> 
> 'return -EINVAL;' I'd suggest.

OK

> 
>>   }
>>   
>>   static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
>> @@ -1476,7 +1479,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>>   			 "Failed to create compatibility class link\n");
>>   #endif
>>   
>> -	i2c_init_recovery(adap);
>> +	res = i2c_init_recovery(adap);
>> +	if (res == -EPROBE_DEFER)
>> +		goto out_link;
> 
> Please move 'i2c_init_recovery' above the class-link creation. It
> shouldn't make a difference but we can skip the extra label and the
> ifdeffery.

Ok. Perhaps I should also move the debug print with the registered 
adapter after calling i2c_init_recovery().

> 
>>   
>>   	/* create pre-declared device nodes */
>>   	of_i2c_register_devices(adap);
>> @@ -1493,6 +1498,11 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>>   
>>   	return 0;
>>   
>> +out_link:
>> +#ifdef CONFIG_I2C_COMPAT
>> +	class_compat_remove_link(i2c_adapter_compat_class, &adap->dev,
>> +				 adap->dev.parent);
>> +#endif
>>   out_reg:
>>   	init_completion(&adap->dev_released);
>>   	device_unregister(&adap->dev);
>> -- 
>> 2.25.1
>>

Do you want me to integrate this patch in the previous one?

Best regards,
Codrin