[PATCH] staging: r8188eu: remove unused members of struct xmit_buf

[PATCH] staging: r8188eu: remove unused members of struct xmit_buf

[Ivan Safonov]

Remove unused members of struct xmit_buf: alloc_sz, ff_hwaddr,
dma_transfer_addr, bpending and last.

Signed-off-by: Ivan Safonov <insafonov@gmail.com>
---
 drivers/staging/rtl8188eu/include/rtw_xmit.h  | 5 -----
 drivers/staging/rtl8188eu/os_dep/xmit_linux.c | 1 -
 2 files changed, 6 deletions(-)

diff --git a/drivers/staging/rtl8188eu/include/rtw_xmit.h b/drivers/staging/rtl8188eu/include/rtw_xmit.h
index 12d16e98176a..3c03987c81a1 100644
--- a/drivers/staging/rtl8188eu/include/rtw_xmit.h
+++ b/drivers/staging/rtl8188eu/include/rtw_xmit.h
@@ -193,14 +193,9 @@ struct xmit_buf {
 	void *priv_data;
 	u16 ext_tag; /*  0: Normal xmitbuf, 1: extension xmitbuf. */
 	u16 flags;
-	u32 alloc_sz;
 	u32  len;
 	struct submit_ctx *sctx;
-	u32	ff_hwaddr;
 	struct urb *pxmit_urb[8];
-	dma_addr_t dma_transfer_addr;	/* (in) dma addr for transfer_buffer */
-	u8 bpending[8];
-	int last[8];
 };
 
 struct xmit_frame {
diff --git a/drivers/staging/rtl8188eu/os_dep/xmit_linux.c b/drivers/staging/rtl8188eu/os_dep/xmit_linux.c
index 017e1d628461..61ced1160951 100644
--- a/drivers/staging/rtl8188eu/os_dep/xmit_linux.c
+++ b/drivers/staging/rtl8188eu/os_dep/xmit_linux.c
@@ -24,7 +24,6 @@ int rtw_os_xmit_resource_alloc(struct adapter *padapter,
 		return _FAIL;
 
 	pxmitbuf->pbuf = PTR_ALIGN(pxmitbuf->pallocated_buf, XMITBUF_ALIGN_SZ);
-	pxmitbuf->dma_transfer_addr = 0;
 
 	for (i = 0; i < 8; i++) {
 		pxmitbuf->pxmit_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
-- 
2.26.2

Re: [PATCH] staging: r8188eu: remove unused members of struct xmit_buf

[Larry Finger]

On 7/12/20 7:38 AM, Ivan Safonov wrote:
> Remove unused members of struct xmit_buf: alloc_sz, ff_hwaddr,
> dma_transfer_addr, bpending and last.
> 
> Signed-off-by: Ivan Safonov <insafonov@gmail.com>
> ---

Have you tested this change? Previously with this driver, an unused quantity was 
removed from one of the structs and the driver failed. Apparently, the alignment 
of some other quantity was affected. I do not think that this change would have 
that affect; however, you should be testing whenever the changes are more than 
cosmetic.

Larry

Re: [PATCH] staging: r8188eu: remove unused members of struct xmit_buf

[Ivan Safonov]

On 7/12/20 8:55 PM, Larry Finger wrote:
> On 7/12/20 7:38 AM, Ivan Safonov wrote:
>> Remove unused members of struct xmit_buf: alloc_sz, ff_hwaddr,
>> dma_transfer_addr, bpending and last.
>>
>> Signed-off-by: Ivan Safonov <insafonov@gmail.com>
>> ---
> 
> Have you tested this change? Previously with this driver, an unused 
> quantity was removed from one of the structs and the driver failed. 
> Apparently, the alignment of some other quantity was affected. I do not 
> think that this change would have that affect; however, you should be 
> testing whenever the changes are more than cosmetic.
> 
> Larry
> 

I have not tested this patch.

Ivan.

Re: [PATCH] staging: r8188eu: remove unused members of struct xmit_buf

[Dan Carpenter]

On Sun, Jul 12, 2020 at 03:38:21PM +0300, Ivan Safonov wrote:
> Remove unused members of struct xmit_buf: alloc_sz, ff_hwaddr,
> dma_transfer_addr, bpending and last.
> 
> Signed-off-by: Ivan Safonov <insafonov@gmail.com>
> ---
>  drivers/staging/rtl8188eu/include/rtw_xmit.h  | 5 -----
>  drivers/staging/rtl8188eu/os_dep/xmit_linux.c | 1 -
>  2 files changed, 6 deletions(-)
> 
> diff --git a/drivers/staging/rtl8188eu/include/rtw_xmit.h b/drivers/staging/rtl8188eu/include/rtw_xmit.h
> index 12d16e98176a..3c03987c81a1 100644
> --- a/drivers/staging/rtl8188eu/include/rtw_xmit.h
> +++ b/drivers/staging/rtl8188eu/include/rtw_xmit.h
> @@ -193,14 +193,9 @@ struct xmit_buf {
>  	void *priv_data;
>  	u16 ext_tag; /*  0: Normal xmitbuf, 1: extension xmitbuf. */
>  	u16 flags;
> -	u32 alloc_sz;
>  	u32  len;
>  	struct submit_ctx *sctx;
> -	u32	ff_hwaddr;
>  	struct urb *pxmit_urb[8];
> -	dma_addr_t dma_transfer_addr;	/* (in) dma addr for transfer_buffer */
> -	u8 bpending[8];
> -	int last[8];
>  };
>  
>  struct xmit_frame {
> diff --git a/drivers/staging/rtl8188eu/os_dep/xmit_linux.c b/drivers/staging/rtl8188eu/os_dep/xmit_linux.c
> index 017e1d628461..61ced1160951 100644
> --- a/drivers/staging/rtl8188eu/os_dep/xmit_linux.c
> +++ b/drivers/staging/rtl8188eu/os_dep/xmit_linux.c
> @@ -24,7 +24,6 @@ int rtw_os_xmit_resource_alloc(struct adapter *padapter,
>  		return _FAIL;
>  
>  	pxmitbuf->pbuf = PTR_ALIGN(pxmitbuf->pallocated_buf, XMITBUF_ALIGN_SZ);

Not related to this patch but kmalloc always returns data which is at
least ARCH_KMALLOC_MINALIGN aligned which is never less than
XMITBUF_ALIGN_SZ (4) so this is a no-op.

> -	pxmitbuf->dma_transfer_addr = 0;

regards,
dan carpenter

Re: [PATCH] staging: r8188eu: remove unused members of struct xmit_buf

[Dan Carpenter]

On Mon, Jul 13, 2020 at 04:16:07PM +0300, Dan Carpenter wrote:
> On Sun, Jul 12, 2020 at 03:38:21PM +0300, Ivan Safonov wrote:
> > Remove unused members of struct xmit_buf: alloc_sz, ff_hwaddr,
> > dma_transfer_addr, bpending and last.
> > 
> > Signed-off-by: Ivan Safonov <insafonov@gmail.com>
> > ---
> >  drivers/staging/rtl8188eu/include/rtw_xmit.h  | 5 -----
> >  drivers/staging/rtl8188eu/os_dep/xmit_linux.c | 1 -
> >  2 files changed, 6 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8188eu/include/rtw_xmit.h b/drivers/staging/rtl8188eu/include/rtw_xmit.h
> > index 12d16e98176a..3c03987c81a1 100644
> > --- a/drivers/staging/rtl8188eu/include/rtw_xmit.h
> > +++ b/drivers/staging/rtl8188eu/include/rtw_xmit.h
> > @@ -193,14 +193,9 @@ struct xmit_buf {
> >  	void *priv_data;
> >  	u16 ext_tag; /*  0: Normal xmitbuf, 1: extension xmitbuf. */
> >  	u16 flags;
> > -	u32 alloc_sz;
> >  	u32  len;
> >  	struct submit_ctx *sctx;
> > -	u32	ff_hwaddr;
> >  	struct urb *pxmit_urb[8];
> > -	dma_addr_t dma_transfer_addr;	/* (in) dma addr for transfer_buffer */
> > -	u8 bpending[8];
> > -	int last[8];
> >  };
> >  
> >  struct xmit_frame {
> > diff --git a/drivers/staging/rtl8188eu/os_dep/xmit_linux.c b/drivers/staging/rtl8188eu/os_dep/xmit_linux.c
> > index 017e1d628461..61ced1160951 100644
> > --- a/drivers/staging/rtl8188eu/os_dep/xmit_linux.c
> > +++ b/drivers/staging/rtl8188eu/os_dep/xmit_linux.c
> > @@ -24,7 +24,6 @@ int rtw_os_xmit_resource_alloc(struct adapter *padapter,
> >  		return _FAIL;
> >  
> >  	pxmitbuf->pbuf = PTR_ALIGN(pxmitbuf->pallocated_buf, XMITBUF_ALIGN_SZ);
> 
> Not related to this patch but kmalloc always returns data which is at
> least ARCH_KMALLOC_MINALIGN aligned which is never less than
> XMITBUF_ALIGN_SZ (4) so this is a no-op.

The alignment in the driver is pretty crazy because it's all unnecessary
and so complicated.  Every allocation is 4 bytes extra so we can align
it later.

Also every buffer is called "pbuf" which stands for pointer to buffer.
"pallocated_buf" is not really useful either.

I tried to look at this to see if we could change the alignment, and
it's complicated because of the naming and the alignment.

regards,
dan carpenter

Re: [PATCH] staging: r8188eu: remove unused members of struct xmit_buf

[Ivan Safonov]

On 7/13/20 5:23 PM, Dan Carpenter wrote:
> On Mon, Jul 13, 2020 at 04:16:07PM +0300, Dan Carpenter wrote:
>> On Sun, Jul 12, 2020 at 03:38:21PM +0300, Ivan Safonov wrote:
>>> Remove unused members of struct xmit_buf: alloc_sz, ff_hwaddr,
>>> dma_transfer_addr, bpending and last.
>>>
>>> Signed-off-by: Ivan Safonov <insafonov@gmail.com>
>>> ---
>>>   drivers/staging/rtl8188eu/include/rtw_xmit.h  | 5 -----
>>>   drivers/staging/rtl8188eu/os_dep/xmit_linux.c | 1 -
>>>   2 files changed, 6 deletions(-)
>>>
>>> diff --git a/drivers/staging/rtl8188eu/include/rtw_xmit.h b/drivers/staging/rtl8188eu/include/rtw_xmit.h
>>> index 12d16e98176a..3c03987c81a1 100644
>>> --- a/drivers/staging/rtl8188eu/include/rtw_xmit.h
>>> +++ b/drivers/staging/rtl8188eu/include/rtw_xmit.h
>>> @@ -193,14 +193,9 @@ struct xmit_buf {
>>>   	void *priv_data;
>>>   	u16 ext_tag; /*  0: Normal xmitbuf, 1: extension xmitbuf. */
>>>   	u16 flags;
>>> -	u32 alloc_sz;
>>>   	u32  len;
>>>   	struct submit_ctx *sctx;
>>> -	u32	ff_hwaddr;
>>>   	struct urb *pxmit_urb[8];
>>> -	dma_addr_t dma_transfer_addr;	/* (in) dma addr for transfer_buffer */
>>> -	u8 bpending[8];
>>> -	int last[8];
>>>   };
>>>   
>>>   struct xmit_frame {
>>> diff --git a/drivers/staging/rtl8188eu/os_dep/xmit_linux.c b/drivers/staging/rtl8188eu/os_dep/xmit_linux.c
>>> index 017e1d628461..61ced1160951 100644
>>> --- a/drivers/staging/rtl8188eu/os_dep/xmit_linux.c
>>> +++ b/drivers/staging/rtl8188eu/os_dep/xmit_linux.c
>>> @@ -24,7 +24,6 @@ int rtw_os_xmit_resource_alloc(struct adapter *padapter,
>>>   		return _FAIL;
>>>   
>>>   	pxmitbuf->pbuf = PTR_ALIGN(pxmitbuf->pallocated_buf, XMITBUF_ALIGN_SZ);
>>
>> Not related to this patch but kmalloc always returns data which is at
>> least ARCH_KMALLOC_MINALIGN aligned which is never less than
>> XMITBUF_ALIGN_SZ (4) so this is a no-op.

4-byte alignment for 8-byte pointer (for example void *priv_data) on 
64-bit arch is an _error_. It’s good that kmalloc (and vmalloc) is 
already aligned to 8 bytes.

> 
> The alignment in the driver is pretty crazy because it's all unnecessary
> and so complicated.  Every allocation is 4 bytes extra so we can align
> it later.
> 
> Also every buffer is called "pbuf" which stands for pointer to buffer.
> "pallocated_buf" is not really useful either.
> 
> I tried to look at this to see if we could change the alignment, and
> it's complicated because of the naming and the alignment.
> 
> regards,
> dan carpenter
> 

I have already fixed 4 places with unnecessary alignment, but, alas, 
there is no great desire to test them on real hardware.

Ivan.

Re: [PATCH] staging: r8188eu: remove unused members of struct xmit_buf

[Larry Finger]

On 7/13/20 1:28 PM, Ivan Safonov wrote:
> On 7/13/20 5:23 PM, Dan Carpenter wrote:
>> On Mon, Jul 13, 2020 at 04:16:07PM +0300, Dan Carpenter wrote:
>>> On Sun, Jul 12, 2020 at 03:38:21PM +0300, Ivan Safonov wrote:
>>>> Remove unused members of struct xmit_buf: alloc_sz, ff_hwaddr,
>>>> dma_transfer_addr, bpending and last.
>>>>
>>>> Signed-off-by: Ivan Safonov <insafonov@gmail.com>
>>>> ---
>>>>   drivers/staging/rtl8188eu/include/rtw_xmit.h  | 5 -----
>>>>   drivers/staging/rtl8188eu/os_dep/xmit_linux.c | 1 -
>>>>   2 files changed, 6 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/rtl8188eu/include/rtw_xmit.h 
>>>> b/drivers/staging/rtl8188eu/include/rtw_xmit.h
>>>> index 12d16e98176a..3c03987c81a1 100644
>>>> --- a/drivers/staging/rtl8188eu/include/rtw_xmit.h
>>>> +++ b/drivers/staging/rtl8188eu/include/rtw_xmit.h
>>>> @@ -193,14 +193,9 @@ struct xmit_buf {
>>>>       void *priv_data;
>>>>       u16 ext_tag; /*  0: Normal xmitbuf, 1: extension xmitbuf. */
>>>>       u16 flags;
>>>> -    u32 alloc_sz;
>>>>       u32  len;
>>>>       struct submit_ctx *sctx;
>>>> -    u32    ff_hwaddr;
>>>>       struct urb *pxmit_urb[8];
>>>> -    dma_addr_t dma_transfer_addr;    /* (in) dma addr for transfer_buffer */
>>>> -    u8 bpending[8];
>>>> -    int last[8];
>>>>   };
>>>>   struct xmit_frame {
>>>> diff --git a/drivers/staging/rtl8188eu/os_dep/xmit_linux.c 
>>>> b/drivers/staging/rtl8188eu/os_dep/xmit_linux.c
>>>> index 017e1d628461..61ced1160951 100644
>>>> --- a/drivers/staging/rtl8188eu/os_dep/xmit_linux.c
>>>> +++ b/drivers/staging/rtl8188eu/os_dep/xmit_linux.c
>>>> @@ -24,7 +24,6 @@ int rtw_os_xmit_resource_alloc(struct adapter *padapter,
>>>>           return _FAIL;
>>>>       pxmitbuf->pbuf = PTR_ALIGN(pxmitbuf->pallocated_buf, XMITBUF_ALIGN_SZ);
>>>
>>> Not related to this patch but kmalloc always returns data which is at
>>> least ARCH_KMALLOC_MINALIGN aligned which is never less than
>>> XMITBUF_ALIGN_SZ (4) so this is a no-op.
> 
> 4-byte alignment for 8-byte pointer (for example void *priv_data) on 64-bit arch 
> is an _error_. It’s good that kmalloc (and vmalloc) is already aligned to 8 bytes.
> 
>>
>> The alignment in the driver is pretty crazy because it's all unnecessary
>> and so complicated.  Every allocation is 4 bytes extra so we can align
>> it later.
>>
>> Also every buffer is called "pbuf" which stands for pointer to buffer.
>> "pallocated_buf" is not really useful either.
>>
>> I tried to look at this to see if we could change the alignment, and
>> it's complicated because of the naming and the alignment.
>>
>> regards,
>> dan carpenter
>>
> 
> I have already fixed 4 places with unnecessary alignment, but, alas, there is no 
> great desire to test them on real hardware.

I have now tested on real hardware and it works fine.

Acked-by: Larry Finger <Larry.Finger@lwfinger.net>

Larry