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

[Kai-Heng Feng]

at 23:55, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> > 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