RE: [PATCH v2 0/2] usb: host: xhci: rcar: fix the quirks setting of XHCI_NO_64BIT_SUPPORT

[Yoshihiro Shimoda]

Hi,

> From: Mathias Nyman
> Sent: Wednesday, June 01, 2016 8:42 PM
> 
> >>> isn't the following enough?
> >>>
> >>> @@ -4886,7 +4886,7 @@ int xhci_gen_setup(struct usb_hcd *hcd, 
> >>> xhci_get_quirks_t get_quirks)
> >>>   xhci->hcc_params2 = readl(>cap_regs->hcc_params2);
> >>>   xhci_print_registers(xhci);
> >>>
> >>> - xhci->quirks = quirks;
> >>> + xhci->quirks |= quirks;
> >>>
> >>>   get_quirks(dev, xhci);
> >>
> >> Thank you for the comment!
> >> You're correct. This also can resolve the issue.
> >> Do you prefer such a simple patch?
> >> At least, I prefer such a simple patch :)
> >
> > I'll defer that to Mathias :-)
> >
> 
> I think that xhci->quirks |= quirks will do as a rc fix.

Thank you very much for the comment and submitting such a patch!

> looks like setting xhci->quirks need some generic cleanup for usb-next.
> Now in 4.7-rc1 we set xhci->quirks before xhci_gen_setup in 
> xhci_priv_init_quirk(),
> and during xhci_gen_setup() when copying module parameters quirk, and when 
> calling
> the get_quirk() callback.
> 
> There is nothing special happening between xhci_priv_init_quirk and when 
> calling
> get_quirks() in xhci_gen_setup() that need quirks to be set in two stages.
> 
> xhci pci drivers only use the get_quirk callback, platform drivers use both.
> 
> Looks like the get_quirk() callback is a legacy thing from when the memory for
> the xhci struct was allocated in xhci_gen_setup, and xhci->quirks could only 
> be
> set after that.
> 
> Now xhci struct is part of the usb_hcd, so we could probably get rid of the 
> get_quirk
> callback alltogether by setting the pci quirks in xhci_pci_setup

I understood it.

Best regards,
Yoshihiro Shimoda

> -Mathias

Re: [PATCH v2 0/2] usb: host: xhci: rcar: fix the quirks setting of XHCI_NO_64BIT_SUPPORT

[Mathias Nyman]

isn't the following enough?

@@ -4886,7 +4886,7 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t 
get_quirks)
xhci->hcc_params2 = readl(>cap_regs->hcc_params2);
xhci_print_registers(xhci);

-   xhci->quirks = quirks;
+   xhci->quirks |= quirks;

get_quirks(dev, xhci);


Thank you for the comment!
You're correct. This also can resolve the issue.
Do you prefer such a simple patch?
At least, I prefer such a simple patch :)


I'll defer that to Mathias :-)



I think that xhci->quirks |= quirks will do as a rc fix.

looks like setting xhci->quirks need some generic cleanup for usb-next.
Now in 4.7-rc1 we set xhci->quirks before xhci_gen_setup in 
xhci_priv_init_quirk(),
and during xhci_gen_setup() when copying module parameters quirk, and when 
calling
the get_quirk() callback.

There is nothing special happening between xhci_priv_init_quirk and when calling
get_quirks() in xhci_gen_setup() that need quirks to be set in two stages.

xhci pci drivers only use the get_quirk callback, platform drivers use both.

Looks like the get_quirk() callback is a legacy thing from when the memory for
the xhci struct was allocated in xhci_gen_setup, and xhci->quirks could only be
set after that.

Now xhci struct is part of the usb_hcd, so we could probably get rid of the 
get_quirk
callback alltogether by setting the pci quirks in xhci_pci_setup

-Mathias  

RE: [PATCH v2 0/2] usb: host: xhci: rcar: fix the quirks setting of XHCI_NO_64BIT_SUPPORT

[Felipe Balbi]

Hi,

Yoshihiro Shimoda  writes:
>> From: Felipe Balbi
>> Sent: Wednesday, June 01, 2016 4:01 PM
>> 
>> Yoshihiro Shimoda  writes:
>> 
>> > I'm afraid but I found a regression of xhci-rcar in v4.7-rc1.
>> > This regression is caused by the following commit:
>> >
>> > commit b1c127ae990bccf0187d741c1695a61e54de1943
>> > Author: Felipe Balbi 
>> > Date:   Fri Apr 22 13:17:16 2016 +0300
>> >
>> > usb: host: xhci: plat: make use of new methods in xhci_plat_priv
>> >
>> > Now that the code has been refactored enough,
>> > switching over to using ->plat_start() and
>> > ->init_quirk() becomes a very simple patch.
>> >
>> > After this patch, there are no further uses for
>> > xhci_plat_type_is() which will be removed in a
>> > follow-up patch.
>> >
>> > Acked-by: Yoshihiro Shimoda 
>> > Signed-off-by: Felipe Balbi 
>> > Signed-off-by: Mathias Nyman 
>> > Signed-off-by: Greg Kroah-Hartman 
>> >
>> > < Overview >
>> > The regression is the quirks flag "XHCI_NO_64BIT_SUPPORT" will be 
>> > overwritten
>> > by xhci_gen_setup(). Then, the driver will not work correctly.
>> >
>> > < Detail >
>> > Since the previous code will do the following, the quirks flag can be set:
>> >
>> > xhci_plat_setup()
>> >  -> xhci_gen_setup(hcd, xhci_plat_quirks);
>> >   -> xhci->quirks = quirks;
>> >-> get_quirks() [This is xhci_plat_quirks]
>> > -> xhci->quirks |= XHCI_NO_64BIT_SUPPORT
>> >
>> > However, after we applied the patch above, the quirks will disappear:
>> >
>> > xhci_plat_setup()
>> >  -> xhci_priv_init_quirk();
>> >   -> xhci_rcar_init_quirk();
>> >-> xhci->quirks |= XHCI_NO_64BIT_SUPPORT
>> >   -> xhci_gen_setup(hcd, xhci_plat_quirks);
>> >-> xhci->quirks = quirks; <- here
>> > -> get_quirks() [This is xhci_plat_quirks]
>> >
>> > So, I submitted incremental patches to resolve this issue like the 
>> > followings:
>> >
>> > xhci_plat_setup()
>> >  -> xhci_priv_init_quirk();
>> >   -> xhci_rcar_init_quirk();
>> >   -> xhci_gen_setup(hcd, xhci_plat_quirks);
>> >-> xhci->quirks = quirks;
>> > -> get_quirks() [This is xhci_plat_quirks]
>> >  -> xhci->quirks |= priv->quirks (XHCI_NO_64BIT_SUPPORT)
>> 
>> isn't the following enough?
>> 
>> @@ -4886,7 +4886,7 @@ int xhci_gen_setup(struct usb_hcd *hcd, 
>> xhci_get_quirks_t get_quirks)
>>  xhci->hcc_params2 = readl(>cap_regs->hcc_params2);
>>  xhci_print_registers(xhci);
>> 
>> -xhci->quirks = quirks;
>> +xhci->quirks |= quirks;
>> 
>>  get_quirks(dev, xhci);
>
> Thank you for the comment!
> You're correct. This also can resolve the issue.
> Do you prefer such a simple patch?
> At least, I prefer such a simple patch :)

I'll defer that to Mathias :-)

> Why I wrote this patch set is I thought I should implement similar
> flow before regression.

understood :-)

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH v2 0/2] usb: host: xhci: rcar: fix the quirks setting of XHCI_NO_64BIT_SUPPORT

[Yoshihiro Shimoda]

Hi Felipe,

> From: Felipe Balbi
> Sent: Wednesday, June 01, 2016 4:01 PM
> 
> Yoshihiro Shimoda  writes:
> 
> > I'm afraid but I found a regression of xhci-rcar in v4.7-rc1.
> > This regression is caused by the following commit:
> >
> > commit b1c127ae990bccf0187d741c1695a61e54de1943
> > Author: Felipe Balbi 
> > Date:   Fri Apr 22 13:17:16 2016 +0300
> >
> > usb: host: xhci: plat: make use of new methods in xhci_plat_priv
> >
> > Now that the code has been refactored enough,
> > switching over to using ->plat_start() and
> > ->init_quirk() becomes a very simple patch.
> >
> > After this patch, there are no further uses for
> > xhci_plat_type_is() which will be removed in a
> > follow-up patch.
> >
> > Acked-by: Yoshihiro Shimoda 
> > Signed-off-by: Felipe Balbi 
> > Signed-off-by: Mathias Nyman 
> > Signed-off-by: Greg Kroah-Hartman 
> >
> > < Overview >
> > The regression is the quirks flag "XHCI_NO_64BIT_SUPPORT" will be 
> > overwritten
> > by xhci_gen_setup(). Then, the driver will not work correctly.
> >
> > < Detail >
> > Since the previous code will do the following, the quirks flag can be set:
> >
> > xhci_plat_setup()
> >  -> xhci_gen_setup(hcd, xhci_plat_quirks);
> >   -> xhci->quirks = quirks;
> >-> get_quirks() [This is xhci_plat_quirks]
> > -> xhci->quirks |= XHCI_NO_64BIT_SUPPORT
> >
> > However, after we applied the patch above, the quirks will disappear:
> >
> > xhci_plat_setup()
> >  -> xhci_priv_init_quirk();
> >   -> xhci_rcar_init_quirk();
> >-> xhci->quirks |= XHCI_NO_64BIT_SUPPORT
> >   -> xhci_gen_setup(hcd, xhci_plat_quirks);
> >-> xhci->quirks = quirks; <- here
> > -> get_quirks() [This is xhci_plat_quirks]
> >
> > So, I submitted incremental patches to resolve this issue like the 
> > followings:
> >
> > xhci_plat_setup()
> >  -> xhci_priv_init_quirk();
> >   -> xhci_rcar_init_quirk();
> >   -> xhci_gen_setup(hcd, xhci_plat_quirks);
> >-> xhci->quirks = quirks;
> > -> get_quirks() [This is xhci_plat_quirks]
> >  -> xhci->quirks |= priv->quirks (XHCI_NO_64BIT_SUPPORT)
> 
> isn't the following enough?
> 
> @@ -4886,7 +4886,7 @@ int xhci_gen_setup(struct usb_hcd *hcd, 
> xhci_get_quirks_t get_quirks)
>   xhci->hcc_params2 = readl(>cap_regs->hcc_params2);
>   xhci_print_registers(xhci);
> 
> - xhci->quirks = quirks;
> + xhci->quirks |= quirks;
> 
>   get_quirks(dev, xhci);

Thank you for the comment!
You're correct. This also can resolve the issue.
Do you prefer such a simple patch?
At least, I prefer such a simple patch :)
Why I wrote this patch set is I thought I should implement similar flow before 
regression.

Best regards,
Yoshihiro Shimoda

Re: [PATCH v2 0/2] usb: host: xhci: rcar: fix the quirks setting of XHCI_NO_64BIT_SUPPORT

[Felipe Balbi]

Yoshihiro Shimoda  writes:

> I'm afraid but I found a regression of xhci-rcar in v4.7-rc1.
> This regression is caused by the following commit:
>
> commit b1c127ae990bccf0187d741c1695a61e54de1943
> Author: Felipe Balbi 
> Date:   Fri Apr 22 13:17:16 2016 +0300
>
> usb: host: xhci: plat: make use of new methods in xhci_plat_priv
> 
> Now that the code has been refactored enough,
> switching over to using ->plat_start() and
> ->init_quirk() becomes a very simple patch.
> 
> After this patch, there are no further uses for
> xhci_plat_type_is() which will be removed in a
> follow-up patch.
> 
> Acked-by: Yoshihiro Shimoda 
> Signed-off-by: Felipe Balbi 
> Signed-off-by: Mathias Nyman 
> Signed-off-by: Greg Kroah-Hartman 
>
> < Overview >
> The regression is the quirks flag "XHCI_NO_64BIT_SUPPORT" will be overwritten
> by xhci_gen_setup(). Then, the driver will not work correctly.
>
> < Detail >
> Since the previous code will do the following, the quirks flag can be set:
>
> xhci_plat_setup()
>  -> xhci_gen_setup(hcd, xhci_plat_quirks);
>   -> xhci->quirks = quirks;
>-> get_quirks() [This is xhci_plat_quirks]
> -> xhci->quirks |= XHCI_NO_64BIT_SUPPORT
>
> However, after we applied the patch above, the quirks will disappear:
>
> xhci_plat_setup()
>  -> xhci_priv_init_quirk();
>   -> xhci_rcar_init_quirk();
>-> xhci->quirks |= XHCI_NO_64BIT_SUPPORT
>   -> xhci_gen_setup(hcd, xhci_plat_quirks);
>-> xhci->quirks = quirks; <- here
> -> get_quirks() [This is xhci_plat_quirks]
>
> So, I submitted incremental patches to resolve this issue like the followings:
>
> xhci_plat_setup()
>  -> xhci_priv_init_quirk();
>   -> xhci_rcar_init_quirk();
>   -> xhci_gen_setup(hcd, xhci_plat_quirks);
>-> xhci->quirks = quirks;
> -> get_quirks() [This is xhci_plat_quirks]
>  -> xhci->quirks |= priv->quirks (XHCI_NO_64BIT_SUPPORT)

isn't the following enough?

@@ -4886,7 +4886,7 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t 
get_quirks)
xhci->hcc_params2 = readl(>cap_regs->hcc_params2);
xhci_print_registers(xhci);
 
-   xhci->quirks = quirks;
+   xhci->quirks |= quirks;
 
get_quirks(dev, xhci);
 
-- 
balbi


signature.asc
Description: PGP signature