* [PATCH] staging: r8188eu: remove unused members of struct xmit_buf
@ 2020-07-12 12:38 Ivan Safonov
2020-07-12 17:55 ` Larry Finger
2020-07-13 13:16 ` Dan Carpenter
0 siblings, 2 replies; 7+ messages in thread
From: Ivan Safonov @ 2020-07-12 12:38 UTC (permalink / raw)
To: Larry Finger
Cc: Greg Kroah-Hartman, Michael Straube, Soumyajit Deb,
Nishka Dasgupta, devel, linux-kernel, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] staging: r8188eu: remove unused members of struct xmit_buf
2020-07-12 12:38 [PATCH] staging: r8188eu: remove unused members of struct xmit_buf Ivan Safonov
@ 2020-07-12 17:55 ` Larry Finger
2020-07-12 21:11 ` Ivan Safonov
2020-07-13 13:16 ` Dan Carpenter
1 sibling, 1 reply; 7+ messages in thread
From: Larry Finger @ 2020-07-12 17:55 UTC (permalink / raw)
To: Ivan Safonov
Cc: Greg Kroah-Hartman, Michael Straube, Soumyajit Deb,
Nishka Dasgupta, devel, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: r8188eu: remove unused members of struct xmit_buf
2020-07-12 17:55 ` Larry Finger
@ 2020-07-12 21:11 ` Ivan Safonov
0 siblings, 0 replies; 7+ messages in thread
From: Ivan Safonov @ 2020-07-12 21:11 UTC (permalink / raw)
To: Larry Finger
Cc: Greg Kroah-Hartman, Michael Straube, Soumyajit Deb,
Nishka Dasgupta, devel, linux-kernel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: r8188eu: remove unused members of struct xmit_buf
2020-07-12 12:38 [PATCH] staging: r8188eu: remove unused members of struct xmit_buf Ivan Safonov
2020-07-12 17:55 ` Larry Finger
@ 2020-07-13 13:16 ` Dan Carpenter
2020-07-13 14:23 ` Dan Carpenter
1 sibling, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2020-07-13 13:16 UTC (permalink / raw)
To: Ivan Safonov
Cc: Larry Finger, devel, Greg Kroah-Hartman, linux-kernel,
Nishka Dasgupta, Soumyajit Deb
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] staging: r8188eu: remove unused members of struct xmit_buf
2020-07-13 13:16 ` Dan Carpenter
@ 2020-07-13 14:23 ` Dan Carpenter
2020-07-13 18:28 ` Ivan Safonov
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2020-07-13 14:23 UTC (permalink / raw)
To: Ivan Safonov
Cc: devel, Greg Kroah-Hartman, linux-kernel, Nishka Dasgupta,
Larry Finger, Soumyajit Deb
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] staging: r8188eu: remove unused members of struct xmit_buf
2020-07-13 14:23 ` Dan Carpenter
@ 2020-07-13 18:28 ` Ivan Safonov
2020-07-13 21:15 ` Larry Finger
0 siblings, 1 reply; 7+ messages in thread
From: Ivan Safonov @ 2020-07-13 18:28 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, Greg Kroah-Hartman, linux-kernel, Nishka Dasgupta,
Larry Finger, Soumyajit Deb
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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] staging: r8188eu: remove unused members of struct xmit_buf
2020-07-13 18:28 ` Ivan Safonov
@ 2020-07-13 21:15 ` Larry Finger
0 siblings, 0 replies; 7+ messages in thread
From: Larry Finger @ 2020-07-13 21:15 UTC (permalink / raw)
To: Ivan Safonov, Dan Carpenter
Cc: devel, Greg Kroah-Hartman, linux-kernel, Nishka Dasgupta,
Soumyajit Deb
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-07-13 21:15 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-07-12 12:38 [PATCH] staging: r8188eu: remove unused members of struct xmit_buf Ivan Safonov
2020-07-12 17:55 ` Larry Finger
2020-07-12 21:11 ` Ivan Safonov
2020-07-13 13:16 ` Dan Carpenter
2020-07-13 14:23 ` Dan Carpenter
2020-07-13 18:28 ` Ivan Safonov
2020-07-13 21:15 ` Larry Finger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox