netdev - Re: [PATCH v4] net: mdiobus: Add a function to deassert reset

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]

Date: Fri, 12 May 2023 10:57:08 +0800
From: Yan Wang <rk.code@...look.com>
To: Simon Horman <simon.horman@...igine.com>
Cc: andrew@...n.ch, hkallweit1@...il.com, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org, linux@...linux.org.uk
Subject: Re: [PATCH v4] net: mdiobus: Add a function to deassert reset



On 5/11/2023 8:40 PM, Simon Horman wrote:
> On Thu, May 11, 2023 at 02:59:09PM +0800, Yan Wang wrote:
>> It is possible to mount multiple sub-devices on the mido bus.
>> The hardware power-on does not necessarily reset these devices.
>> The device may be in an uncertain state, causing the device's ID
>> to not be scanned.
>>
>> So,before adding a reset to the scan, make sure the device is in
>> normal working mode.
>>
>> Reported-by: kernel test robot <lkp@...el.com>
>> Link: https://lore.kernel.org/oe-kbuild-all/202305101702.4xW6vT72-lkp@intel.com/
>> Signed-off-by: Yan Wang <rk.code@...look.com>
> ...
>
>> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
>> index 1183ef5e203e..9d7df6393059 100644
>> --- a/drivers/net/mdio/fwnode_mdio.c
>> +++ b/drivers/net/mdio/fwnode_mdio.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/of.h>
>>   #include <linux/phy.h>
>>   #include <linux/pse-pd/pse.h>
>> +#include <linux/gpio/consumer.h>
>>   
>>   MODULE_AUTHOR("Calvin Johnson <calvin.johnson@....nxp.com>");
>>   MODULE_LICENSE("GPL");
>> @@ -57,6 +58,35 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode)
>>   	return register_mii_timestamper(arg.np, arg.args[0]);
>>   }
>>   
>> +static void fwnode_mdiobus_pre_enable_phy(struct fwnode_handle *fwnode)
>> +{
>> +	struct gpio_desc *reset;
>> +	unsigned int reset_assert_delay;
>> +	unsigned int reset_deassert_delay;
> nit: Please arrange local variables for networking code in reverse xmas
>       tree order - longest line to shortest.
>
>> +
>> +	reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_LOW, NULL);
>> +	if (IS_ERR(reset)) {
>> +		if (PTR_ERR(reset) == -EPROBE_DEFER)
>> +			pr_debug("%pOFn: %s: GPIOs not yet available, retry later\n",
>> +				 to_of_node(fwnode), __func__);
>> +		else
>> +			pr_err("%pOFn: %s: Can't get reset line property\n",
>> +			       to_of_node(fwnode), __func__);
>> +
>> +		return;
>> +	}
>> +	fwnode_property_read_u32(fwnode, "reset-assert-us",
>> +				 &reset_assert_delay);
>> +	fwnode_property_read_u32(fwnode, "reset-deassert-us",
>> +				 &reset_deassert_delay);
> Does the return value of fwnode_property_read_u32() need to be
> checked for errors?
>
>> +	gpiod_set_value_cansleep(reset, 1);
>> +	fsleep(reset_assert_delay);
>> +	gpiod_set_value_cansleep(reset, 0);
>> +	fsleep(reset_deassert_delay);
>> +	/*Release phy's reset line, mdiobus_register_gpiod() need to request it*/
> nit:
>
> 	/* Release phy's reset line, mdiobus_register_gpiod() needs to
> 	 * request it.
> 	 */

Thank you for your suggestion.

Best Regards
>> +	gpiod_put(reset);
>> +}
>> +
>>   int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
>>   				       struct phy_device *phy,
>>   				       struct fwnode_handle *child, u32 addr)
>> @@ -119,6 +149,8 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
>>   	u32 phy_id;
>>   	int rc;
>>   
>> +	fwnode_mdiobus_pre_enable_phy(child);
>> +
>>   	psec = fwnode_find_pse_control(child);
>>   	if (IS_ERR(psec))
>>   		return PTR_ERR(psec);
>> -- 
>> 2.17.1
>>
>>