Re: [PATCH v9 5/8] usb: misc: qcom_eud: improve enable_store API

[Elson Serrao <elson.serrao@oss.qualcomm.com>]

On 9/29/2026 2:43 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] usb: qcom_eud: uninitialized software state blocks disabling EUD on boot
> --
> 
> commit 16556b4a5664eacf6eb75fc1fe5031d31c087dc0
> Author: Elson Serrao <elson.serrao@oss.qualcomm.com>
> 
> usb: misc: qcom_eud: improve enable_store API
> 
> This commit adds a state check in enable_store() to skip redundant hardware
> operations when the EUD is already in the desired state. It also improves
> error handling and logging when enabling or disabling the EUD.
> 
>> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
>> index 48e474bdf818c..e227d05cfecd4 100644
>> --- a/drivers/usb/misc/qcom_eud.c
>> +++ b/drivers/usb/misc/qcom_eud.c
> [ ... ]
>> @@ -164,18 +164,27 @@ static ssize_t enable_store(struct device *dev,
>>  	if (kstrtobool(buf, &enable))
>>  		return -EINVAL;
>>  
>> +	/* Skip operation if already in desired state */
>> +	if (chip->enabled == enable)
>> +		return count;
> 
> [Severity: Medium]
> Will this early exit prevent users from disabling the hardware if the
> bootloader left the EUD enabled?
> 

A bootloader-enabled EUD has never been supported; the driver has
always assumed EUD is disabled at probe. Supporting it would need more
than syncing chip->enabled from the register. The driver would also have
to take the PHY votes and bring the role-switch state into line for the
active path. That is a separate feature and outside the scope of this
series.

Thanks
Elson

> Since eud_probe() initializes chip->enabled to false (typically via
> devm_kzalloc), it does not verify or synchronize the software state with
> the actual hardware state by reading the hardware control registers (e.g.,
> EUD_REG_CSR_EUD_EN).
> 
> If the bootloader leaves the EUD enabled, the hardware is active but
> chip->enabled is false.
> 
> If a user then writes 0 to the sysfs attribute to explicitly disable the
> EUD, this check evaluates to true (false == false) and returns early.
> 
> This bypasses the disable_eud() hardware routine, preventing the hardware
> from being disabled and potentially impacting power consumption or role
> switching.
>