guidance on struct alignment for rtl8192cu driver

guidance on struct alignment for rtl8192cu driver

[Jason Andrews]

I'm using an ASUS USB N13 on an ARM platform with the rtl8192cu driver.
Linux kernel is 3.10 so I probably don't have the latest and greatest driver.

When I booted I got an ARM alignment trap caused by the driver.

I determined the cause was the 1st argument to spin_lock_irqsave() has an unaligned address.

By trial-and-error I found that if I edit wifi.h and insert 2 dummy bytes into the rtl_priv struct just above priv (last variable) the locks work and the driver works fine.

What is the recommended way to make sure the last variable in the rtl_priv struct (u8 priv[0]) is aligned on a 4 byte boundary so the driver works on ARM machines?

Regards,
Jason

Re: guidance on struct alignment for rtl8192cu driver

[Larry Finger]

On 09/14/2013 12:36 AM, Jason Andrews wrote:
> I'm using an ASUS USB N13 on an ARM platform with the rtl8192cu driver.
> Linux kernel is 3.10 so I probably don't have the latest and greatest driver.
>
> When I booted I got an ARM alignment trap caused by the driver.
>
> I determined the cause was the 1st argument to spin_lock_irqsave() has an unaligned address.
>
> By trial-and-error I found that if I edit wifi.h and insert 2 dummy bytes into the rtl_priv struct just above priv (last variable) the locks work and the driver works fine.
>
> What is the recommended way to make sure the last variable in the rtl_priv struct (u8 priv[0]) is aligned on a 4 byte boundary so the driver works on ARM machines?

There are a lot of improvements for this driver in 3.11. The backports release 
has that code. In addition, I am currently working at improving the power 
management for 3.13.

The presence of unaligned variables that cause alignment traps on ARM does not 
surprise me as I test only on x86 and ppc architectures. I now own a Raspberry 
Pi and I will soon be testing with it as well.

What does surprise me is that the first argument in all the calls to 
spin_lock_irqsave() are contained within the rtl_locks struct and everything 
there should be aligned. Perhaps some ARM expert will know why aligning the last 
item in the rtl_priv struct fixes the problem.

As far as I know, the proper way to do a 4-byte alignment is as in the following 
patch:

Index: wireless-testing-save/drivers/net/wireless/rtlwifi/wifi.h
===================================================================
--- wireless-testing-save.orig/drivers/net/wireless/rtlwifi/wifi.h
+++ wireless-testing-save/drivers/net/wireless/rtlwifi/wifi.h
@@ -2057,7 +2057,7 @@ struct rtl_priv {
  	  that it points to the data allocated
  	  beyond  this structure like:
  	  rtl_pci_priv or rtl_usb_priv */
-	u8 priv[0];
+	u8 __aligned(4) priv[0];
  };

  #define rtl_priv(hw)           (((struct rtl_priv *)(hw)->priv))

Larry

Re: guidance on struct alignment for rtl8192cu driver

[Seth Forshee]

On Sat, Sep 14, 2013 at 09:08:34AM -0500, Larry Finger wrote:
> On 09/14/2013 12:36 AM, Jason Andrews wrote:
> >I'm using an ASUS USB N13 on an ARM platform with the rtl8192cu driver.
> >Linux kernel is 3.10 so I probably don't have the latest and greatest driver.
> >
> >When I booted I got an ARM alignment trap caused by the driver.
> >
> >I determined the cause was the 1st argument to spin_lock_irqsave() has an unaligned address.
> >
> >By trial-and-error I found that if I edit wifi.h and insert 2 dummy bytes into the rtl_priv struct just above priv (last variable) the locks work and the driver works fine.
> >
> >What is the recommended way to make sure the last variable in the rtl_priv struct (u8 priv[0]) is aligned on a 4 byte boundary so the driver works on ARM machines?
> 
> There are a lot of improvements for this driver in 3.11. The
> backports release has that code. In addition, I am currently working
> at improving the power management for 3.13.
> 
> The presence of unaligned variables that cause alignment traps on
> ARM does not surprise me as I test only on x86 and ppc
> architectures. I now own a Raspberry Pi and I will soon be testing
> with it as well.
> 
> What does surprise me is that the first argument in all the calls to
> spin_lock_irqsave() are contained within the rtl_locks struct and
> everything there should be aligned. Perhaps some ARM expert will
> know why aligning the last item in the rtl_priv struct fixes the
> problem.

Depending on architecture version and configuration ARM may or may not
allow unaligned accesses. Even when allowed there is a cost though, so
it's better to properly align the data. In the past this would have
always meant 4-byte alignment, but my ARM experience is a bit dated now
and I don't know about 64-bit ARM. That variable-size array probably
only has byte alignment.

> As far as I know, the proper way to do a 4-byte alignment is as in
> the following patch:
> 
> Index: wireless-testing-save/drivers/net/wireless/rtlwifi/wifi.h
> ===================================================================
> --- wireless-testing-save.orig/drivers/net/wireless/rtlwifi/wifi.h
> +++ wireless-testing-save/drivers/net/wireless/rtlwifi/wifi.h
> @@ -2057,7 +2057,7 @@ struct rtl_priv {
>  	  that it points to the data allocated
>  	  beyond  this structure like:
>  	  rtl_pci_priv or rtl_usb_priv */
> -	u8 priv[0];
> +	u8 __aligned(4) priv[0];
>  };

__attribute__((aligned)) might be a safer bet, as this will align it to
the largest alignment that could possibly be needed.

Seth

Re: guidance on struct alignment for rtl8192cu driver

[Larry Finger]

On 09/16/2013 09:35 AM, Seth Forshee wrote:
> On Sat, Sep 14, 2013 at 09:08:34AM -0500, Larry Finger wrote:
>> On 09/14/2013 12:36 AM, Jason Andrews wrote:
>>> I'm using an ASUS USB N13 on an ARM platform with the rtl8192cu driver.
>>> Linux kernel is 3.10 so I probably don't have the latest and greatest driver.
>>>
>>> When I booted I got an ARM alignment trap caused by the driver.
>>>
>>> I determined the cause was the 1st argument to spin_lock_irqsave() has an unaligned address.
>>>
>>> By trial-and-error I found that if I edit wifi.h and insert 2 dummy bytes into the rtl_priv struct just above priv (last variable) the locks work and the driver works fine.
>>>
>>> What is the recommended way to make sure the last variable in the rtl_priv struct (u8 priv[0]) is aligned on a 4 byte boundary so the driver works on ARM machines?
>>
>> There are a lot of improvements for this driver in 3.11. The
>> backports release has that code. In addition, I am currently working
>> at improving the power management for 3.13.
>>
>> The presence of unaligned variables that cause alignment traps on
>> ARM does not surprise me as I test only on x86 and ppc
>> architectures. I now own a Raspberry Pi and I will soon be testing
>> with it as well.
>>
>> What does surprise me is that the first argument in all the calls to
>> spin_lock_irqsave() are contained within the rtl_locks struct and
>> everything there should be aligned. Perhaps some ARM expert will
>> know why aligning the last item in the rtl_priv struct fixes the
>> problem.
>
> Depending on architecture version and configuration ARM may or may not
> allow unaligned accesses. Even when allowed there is a cost though, so
> it's better to properly align the data. In the past this would have
> always meant 4-byte alignment, but my ARM experience is a bit dated now
> and I don't know about 64-bit ARM. That variable-size array probably
> only has byte alignment.
>
>> As far as I know, the proper way to do a 4-byte alignment is as in
>> the following patch:
>>
>> Index: wireless-testing-save/drivers/net/wireless/rtlwifi/wifi.h
>> ===================================================================
>> --- wireless-testing-save.orig/drivers/net/wireless/rtlwifi/wifi.h
>> +++ wireless-testing-save/drivers/net/wireless/rtlwifi/wifi.h
>> @@ -2057,7 +2057,7 @@ struct rtl_priv {
>>   	  that it points to the data allocated
>>   	  beyond  this structure like:
>>   	  rtl_pci_priv or rtl_usb_priv */
>> -	u8 priv[0];
>> +	u8 __aligned(4) priv[0];
>>   };
>
> __attribute__((aligned)) might be a safer bet, as this will align it to
> the largest alignment that could possibly be needed.

Seth,

Thanks for the help. So far, I have not heard if my original patch helps or not. 
When, and if, I hear, I will use your suggestion for the final patch.

Larry

Re: guidance on struct alignment for rtl8192cu driver

[Emmanuel Grumbach]

>>> Index: wireless-testing-save/drivers/net/wireless/rtlwifi/wifi.h
>>> ===================================================================
>>> --- wireless-testing-save.orig/drivers/net/wireless/rtlwifi/wifi.h
>>> +++ wireless-testing-save/drivers/net/wireless/rtlwifi/wifi.h
>>> @@ -2057,7 +2057,7 @@ struct rtl_priv {
>>>           that it points to the data allocated
>>>           beyond  this structure like:
>>>           rtl_pci_priv or rtl_usb_priv */
>>> -       u8 priv[0];
>>> +       u8 __aligned(4) priv[0];
>>>   };
>>
>>
>> __attribute__((aligned)) might be a safer bet, as this will align it to
>> the largest alignment that could possibly be needed.

Or copy the code from mac80211.h:

u8 drv_priv[0] __aligned(sizeof(void *));

I did the same in iwlwifi.
Note the new way to add the __aligned thing. Joe will tell you that is
better than __attribute__ blablabla

Re: guidance on struct alignment for rtl8192cu driver

[Larry Finger]

On 09/16/2013 02:29 PM, Emmanuel Grumbach wrote:
>>>> Index: wireless-testing-save/drivers/net/wireless/rtlwifi/wifi.h
>>>> ===================================================================
>>>> --- wireless-testing-save.orig/drivers/net/wireless/rtlwifi/wifi.h
>>>> +++ wireless-testing-save/drivers/net/wireless/rtlwifi/wifi.h
>>>> @@ -2057,7 +2057,7 @@ struct rtl_priv {
>>>>            that it points to the data allocated
>>>>            beyond  this structure like:
>>>>            rtl_pci_priv or rtl_usb_priv */
>>>> -       u8 priv[0];
>>>> +       u8 __aligned(4) priv[0];
>>>>    };
>>>
>>>
>>> __attribute__((aligned)) might be a safer bet, as this will align it to
>>> the largest alignment that could possibly be needed.
>
> Or copy the code from mac80211.h:
>
> u8 drv_priv[0] __aligned(sizeof(void *));
>
> I did the same in iwlwifi.
> Note the new way to add the __aligned thing. Joe will tell you that is
> better than __attribute__ blablabla

Thanks. I had noticed that checkpatch.pl complains about the __attribute 
construction.

Larry

RE: guidance on struct alignment for rtl8192cu driver

[Jason Andrews]

> -----Original Message-----
> From: Larry Finger [mailto:larry.finger@gmail.com] On Behalf Of Larry
> Finger
> Sent: Monday, September 16, 2013 9:40 PM
> To: Emmanuel Grumbach
> Cc: Seth Forshee; Jason Andrews; linux-wireless@vger.kernel.org
> Subject: Re: guidance on struct alignment for rtl8192cu driver
> 
> On 09/16/2013 02:29 PM, Emmanuel Grumbach wrote:
> >>>> Index: wireless-testing-save/drivers/net/wireless/rtlwifi/wifi.h
> >>>>
> ===================================================================
> >>>> --- wireless-testing-save.orig/drivers/net/wireless/rtlwifi/wifi.h
> >>>> +++ wireless-testing-save/drivers/net/wireless/rtlwifi/wifi.h
> >>>> @@ -2057,7 +2057,7 @@ struct rtl_priv {
> >>>>            that it points to the data allocated
> >>>>            beyond  this structure like:
> >>>>            rtl_pci_priv or rtl_usb_priv */
> >>>> -       u8 priv[0];
> >>>> +       u8 __aligned(4) priv[0];
> >>>>    };
> >>>
> >>>
> >>> __attribute__((aligned)) might be a safer bet, as this will align
> it to
> >>> the largest alignment that could possibly be needed.
> >
> > Or copy the code from mac80211.h:
> >
> > u8 drv_priv[0] __aligned(sizeof(void *));
> >
> > I did the same in iwlwifi.
> > Note the new way to add the __aligned thing. Joe will tell you that
> is
> > better than __attribute__ blablabla
> 
> Thanks. I had noticed that checkpatch.pl complains about the
> __attribute
> construction.
> 
> Larry
> 

Larry,

I confirmed that my original alignment error is properly solved by:

    u8 drv_priv[0] __aligned(sizeof(void *));

The driver is now working for my ARM system.

Regards,
Jason