serious doubts concerning a7d57abcc8a5bdeb53bbf8e87558e8e0a2c2a29d ("xhci: workaround CSS timeout on AMD SNPS 3.0 xHC")

serious doubts concerning a7d57abcc8a5bdeb53bbf8e87558e8e0a2c2a29d ("xhci: workaround CSS timeout on AMD SNPS 3.0 xHC")

[Greg Kroah-Hartman]

On Mon, Dec 17, 2018 at 01:33:05PM +0100, Oliver Neukum wrote:
> Hi,
> 
> this patch contains the following section:
> 
> index 260b259b72bc..c3515bad5dbb 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1850,6 +1850,7 @@ struct xhci_hcd {
>  #define XHCI_ZERO_64B_REGS     BIT_ULL(32)
>  #define XHCI_DEFAULT_PM_RUNTIME_ALLOW  BIT_ULL(33)
>  #define XHCI_RESET_PLL_ON_DISCONNECT   BIT_ULL(34)
> +#define XHCI_SNPS_BROKEN_SUSPEND    BIT_ULL(35)
>  
>         unsigned int            num_active_eps;
>         unsigned int            limit_active_eps;
> @@ -1879,6 +1880,8 @@ struct xhci_hcd {
>         void                    *dbc;
>         /* platform-specific data -- must come last */
>         unsigned long           priv[0] __aligned(sizeof(s64));
> +       /* Broken Suspend flag for SNPS Suspend resume issue */
> +       u8                      broken_suspend;
>  };
>  
>  /* Platform specific overrides to generic XHCI hc_driver ops */
> 
> The placement of the broken_suspend flag seems to break every
> usage of the priv member of struct xhci_hcd.
> Are you sure that is a good idea?

Ugh, ick, no, that's totally wrong :(

How did this work?  I guess no one tried this type of host controller on
a platform xhci driver?

Sandeep or oliver, , want to send a fixup patch?
And shouldn't that just be a bool or a bitflag?

thanks,

greg k-h

serious doubts concerning a7d57abcc8a5bdeb53bbf8e87558e8e0a2c2a29d ("xhci: workaround CSS timeout on AMD SNPS 3.0 xHC")

[Oliver Neukum]

Hi,

this patch contains the following section:

index 260b259b72bc..c3515bad5dbb 100644

The placement of the broken_suspend flag seems to break every
usage of the priv member of struct xhci_hcd.
Are you sure that is a good idea?

	Regards
		Oliver

--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1850,6 +1850,7 @@ struct xhci_hcd {
 #define XHCI_ZERO_64B_REGS     BIT_ULL(32)
 #define XHCI_DEFAULT_PM_RUNTIME_ALLOW  BIT_ULL(33)
 #define XHCI_RESET_PLL_ON_DISCONNECT   BIT_ULL(34)
+#define XHCI_SNPS_BROKEN_SUSPEND    BIT_ULL(35)
 
        unsigned int            num_active_eps;
        unsigned int            limit_active_eps;
@@ -1879,6 +1880,8 @@ struct xhci_hcd {
        void                    *dbc;
        /* platform-specific data -- must come last */
        unsigned long           priv[0] __aligned(sizeof(s64));
+       /* Broken Suspend flag for SNPS Suspend resume issue */
+       u8                      broken_suspend;
 };
 
 /* Platform specific overrides to generic XHCI hc_driver ops */