Re: kern/54486: athn driver panics on arm

[Andreas Gustafsson <gson%gson.org@localhost>]

The following reply was made to PR kern/54486; it has been noted by GNATS.

From: Andreas Gustafsson <gson%gson.org@localhost>
To: matthew green <mrg%eterna.com.au@localhost>, gnats-bugs%netbsd.org@localhost
Cc: Rin Okuyama <rokuyama%rk.phys.keio.ac.jp@localhost>, Christos Zoulas <christos%astron.com@localhost>, maya%NetBSD.org@localhost
Subject: Re: kern/54486: athn driver panics on arm
Date: Sat, 24 Aug 2019 11:20:17 +0300

 matthew green wrote:
 > >  Looks like gcc is optimizing the memcpy() call on line 2528 of
 > >  src/sys/dev/usb/if_athn_usb.c into an unaligned 32-bit store:
 > >  
 > >                  txf = (struct ar_tx_frame *)&htc[1];
 > >                  memset(txf, 0, sizeof(*txf));
 > >                  txf->data_type = AR_HTC_NORMAL;
 > >                  txf->node_idx = sta_index;
 > >                  txf->vif_idx = 0;
 > >                  txf->tid = tid;
 > >  
 > >  The fields at the beginning and end of *txf are being assigned to, so
 > >  only the middle part really needs the memset()ting, and gcc apparently
 > >  figures the way to do that is to store a 32-bit word of zeros there,
 > >  at an unaligned address.
 > >  
 > >  I would not be surprised if this optimizaion is also breaking other
 > >  parts of the arm kernel.  Does anyone know the right way to fix this?
 > 
 > this looks similar to the problem worked around in if_axe.c:
 > 
 >    http://mail-index.netbsd.org/source-changes/2019/01/06/msg102106.html
 > 
 > i forget the status of a real fix.  there's some issue with
 > unaligned on v6/v7, IIRC.
 
 I found some related discussion in the port-arm archives, starting at:
 
   http://mail-index.netbsd.org/port-arm/2019/01/06/msg005357.html
 
 My understanding of that discussion is that there are two alternative
 correct fixes: either apply the patch from the above message to add
 -mno-unaligned-access to CFLAGS, or stop using PMAP_NOCACHE in
 bus_dma.c as mentioned in:
 
   http://mail-index.netbsd.org/port-arm/2019/01/06/msg005358.html
 
 The problem is that *neither* has been done.  Since the patch is less
 likely to have bad side effects, I think we should apply the patch
 now, and if or when the pmap change is ever made, the patch can be
 reverted.
 
 > but i think this one is a code bug.
 > 
 > the axe case has a structure with two uint16_t's in it, and
 > assumes they will be 32 bit aligned.  that's wrong.
 > 
 > however, in this case, struct ar_tx_frame has 4 x uint8_ts
 > and then a uint32_t.
 
 Sorry, I quoted the code around the wrong memset() call in the PR.
 The actual code around line 2528 is:
 
                 txm = (struct ar_tx_mgmt *)&htc[1];
                 memset(txm, 0, sizeof(*txm));
                 txm->node_idx = sta_index;
                 txm->vif_idx = 0;
                 txm->key_idx = 0xff;
                 frm = (uint8_t *)&txm[1];
 
 which involves struct ar_tx_mgmt rather than ar_tx_frame.
 
 > that means the compiler is probably right to assume that a
 > pointer to this structure will be aligned for uint32_t, so
 > in this case, the compiler seems to be not wrong, and we 
 > should fix the code.
 
 The pointer to the structure is aligned.  The lack of alignment arises
 from gcc optimizing a memset of the full structure into a store to
 an unaligned subset of the structure fields.
 
 > could you try this patch?
 
 I'm not really interested in work-arounds for individual drivers.
 Unless we fix the general problem, it's going to continue to bite us
 again and again.
 -- 
 Andreas Gustafsson, gson%gson.org@localhost