Re: [PATCH v2 2/2] USB: storage: ums-realtek: Make auto-delink support optionally

[Kai-Heng Feng <kai.heng.feng@canonical.com>]

at 23:55, Alan Stern <stern@rowland.harvard.edu> wrote:

> On Mon, 26 Aug 2019, Kai-Heng Feng wrote:
>
>> Auto-delink requires writing special registers to ums-realtek device.
>> Unconditionally enable auto-delink may break newer devices.
>>
>> So only enable auto-delink by default for the original three IDs,
>> 0x0138, 0x0158 and 0x0159.
>>
>> Realtek is working on a patch to properly support auto-delink for other
>> IDs.
>>
>> BugLink: https://bugs.launchpad.net/bugs/1838886
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> ---
>> v2:
>> - Use auto_delink_support instead of auto_delink_enable.
>>
>> drivers/usb/storage/realtek_cr.c | 24 +++++++++++++++++++-----
>>  1 file changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/storage/realtek_cr.c  
>> b/drivers/usb/storage/realtek_cr.c
>> index beaffac805af..b304cca7c4fa 100644
>> --- a/drivers/usb/storage/realtek_cr.c
>> +++ b/drivers/usb/storage/realtek_cr.c
>> @@ -40,6 +40,10 @@ static int auto_delink_en = 1;
>>  module_param(auto_delink_en, int, S_IRUGO | S_IWUSR);
>>  MODULE_PARM_DESC(auto_delink_en, "auto delink mode (0=firmware, 1=software [default])");
>>
>> +static int auto_delink_support = -1;
>> +module_param(auto_delink_support, int, S_IRUGO | S_IWUSR);
>> +MODULE_PARM_DESC(auto_delink_support, "enable auto delink (-1=auto  
>> [default], 0=disable, 1=enable)");
>> +
>>  #ifdef CONFIG_REALTEK_AUTOPM
>>  static int ss_en = 1;
>>  module_param(ss_en, int, S_IRUGO | S_IWUSR);
>> @@ -996,12 +1000,22 @@ static int init_realtek_cr(struct us_data *us)
>>  			goto INIT_FAIL;
>>  	}
>>
>> -	if (CHECK_FW_VER(chip, 0x5888) || CHECK_FW_VER(chip, 0x5889) ||
>> -	    CHECK_FW_VER(chip, 0x5901))
>> -		SET_AUTO_DELINK(chip);
>> -	if (STATUS_LEN(chip) == 16) {
>> -		if (SUPPORT_AUTO_DELINK(chip))
>> +	if (auto_delink_support == -1) {
>> +		if (CHECK_PID(chip, 0x0138) || CHECK_PID(chip, 0x0158) ||
>> +		    CHECK_PID(chip, 0x0159))
>> +			auto_delink_support = 1;
>> +		else
>> +			auto_delink_support = 0;
>> +	}
>
> What will happen if somebody has two Realtek devices plugged in, where
> one of them has an old product ID and the other has a new one?  You
> shouldn't change the value of the module parameter like this.

You are right, I didn’t think of that.

>
>> +
>> +	if (auto_delink_support) {
>> +		if (CHECK_FW_VER(chip, 0x5888) || CHECK_FW_VER(chip, 0x5889) ||
>> +				CHECK_FW_VER(chip, 0x5901))
>>  			SET_AUTO_DELINK(chip);
>> +		if (STATUS_LEN(chip) == 16) {
>> +			if (SUPPORT_AUTO_DELINK(chip))
>> +				SET_AUTO_DELINK(chip);
>> +		}
>>  	}
>>  #ifdef CONFIG_REALTEK_AUTOPM
>>  	if (ss_en)
>
> Instead of adding a new module parameter, how about just changing the
> driver's behavior?  If a chip doesn't have the right product ID, don't
> enable auto_delink regardless of what the module parameter is set to.

Ok, I think whitelist is a better approach until Realtek comes up with a  
long term solution.

Kai-Heng

>
> Alan Stern