Remove unused members of struct xmit_buf: alloc_sz, ff_hwaddr,
dma_transfer_addr, bpending and last.
Signed-off-by: Ivan Safonov <[email protected]>
---
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
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 <[email protected]>
> ---
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
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 <[email protected]>
>> ---
>
> 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.
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
>>> ---
>>> 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.
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 <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
Larry