linux-kernel - Re: [PATCH v3 1/1] i2c: busses: i2c-mv64xxx: fix arb-loss i2c lock

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: <qki2w47cfu74j6tuwz363wgsleypbkgkejtzbk7zfktjxlbmqp@h3cejf2gnccb>
Date: Thu, 4 Jan 2024 12:04:51 +0100
From: Andi Shyti <andi.shyti@...nel.org>
To: Elad Nachman <enachman@...vell.com>
Cc: gregory.clement@...tlin.com, linux-i2c@...r.kernel.org, 
	linux-kernel@...r.kernel.org, cyuval@...vell.com
Subject: Re: [PATCH v3 1/1] i2c: busses: i2c-mv64xxx: fix arb-loss i2c lock

Hi Elad,

first of all it's very nice to see code so well documented :-)

Thanks for the effort you put in it.

...

> +	/*
> +	 * Start with arbitration lost soft reset enabled as to false.
> +	 * Try to locate the necessary items in the device tree to
> +	 * make this feature work.
> +	 * Only after we verify that the device tree contains all of
> +	 * the needed information and that it is sound and usable,
> +	 * then we enable this flag.
> +	 * This information should be defined, but the driver maintains
> +	 * backward compatibility with old dts files, so it will not fail
> +	 * the probe in case these are missing.
> +	 */
> +	drv_data->soft_reset = false;
> +	pc = pinctrl_get(&pd->dev);

I'm not against using devm_pinctrl_get(), in my previous comments
I was questioning whether it should be placed in the probe
function (as you did).

Placed there, iirc, it needed explicit release. Here, I don't
think it does if you use the managed version.

> +	if (!IS_ERR(pc)) {
> +		drv_data->pc = pc;
> +		drv_data->i2c_mpp_state =
> +			pinctrl_lookup_state(pc, "default");
> +		drv_data->i2c_gpio_state =
> +			pinctrl_lookup_state(pc, "gpio");
> +		drv_data->scl_gpio =
> +			of_get_named_gpio(pd->dev.of_node, "scl-gpios", 0);
> +		drv_data->sda_gpio =
> +			of_get_named_gpio(pd->dev.of_node, "sda-gpios", 0);

please use devm_gpio_get(...).

> +		if (!IS_ERR(drv_data->i2c_gpio_state) &&
> +		    !IS_ERR(drv_data->i2c_mpp_state) &&
> +		    gpio_is_valid(drv_data->scl_gpio) &&
> +		    gpio_is_valid(drv_data->sda_gpio)) {

...

> +	if (!IS_ERR(drv_data->pc))
> +		pinctrl_put(drv_data->pc);
> +	if (drv_data->soft_reset) {
> +		devm_gpiod_put(drv_data->adapter.dev.parent,
> +			       gpio_to_desc(drv_data->scl_gpio));
> +		devm_gpiod_put(drv_data->adapter.dev.parent,
> +			       gpio_to_desc(drv_data->sda_gpio));

if it's managed it doesn't need to be explicitely removed.

Thanks,
Andi