About improving the qlge Ethernet driver by following drivers/staging/qlge/TODO

About improving the qlge Ethernet driver by following drivers/staging/qlge/TODO

[Coiby Xu]

Hi Benjamin,

As you have known, I'm working on improving drivers/staging/qlge. I'm
not sure if I correctly understand some TODO items. Since you wrote 
the TODO list, could you explain some of the items or comment on the
corresponding fix for me?

> * while in that area, using two 8k buffers to store one 9k frame is a poor
>   choice of buffer size.

Currently, LARGE_BUFFER_MAX_SIZE is defined as 8192. How about we simply
changing LARGE_BUFFER_MAX_SIZE to 4096? This is what 
drivers/net/ethernet/intel/e1000 does for jumbo frame right now.


> * in the "chain of large buffers" case, the driver uses an skb allocated with
>   head room but only puts data in the frags.

Do you suggest implementing the copybreak feature which exists for e1000 for 
this driver, i.e., allocing a sk_buff and coping the header buffer into it?


> * fix weird line wrapping (all over, ex. the ql_set_routing_reg() calls in
>   qlge_set_multicast_list()).

This issue of weird line wrapping is supposed to be all over. But I can
only find the ql_set_routing_reg() calls in qlge_set_multicast_list have
this problem,

			if (qlge_set_routing_reg
			    (qdev, RT_IDX_PROMISCUOUS_SLOT, RT_IDX_VALID, 1)) {

I can't find other places where functions calls put square and arguments
in the new line. Could you give more hints?

Thanks!

-- 
Best regards,
Coiby

Re: About improving the qlge Ethernet driver by following drivers/staging/qlge/TODO

[Benjamin Poirier]

[-- Attachment #1: Type: text/plain, Size: 3878 bytes --]

On 2021-05-04 21:14 +0800, Coiby Xu wrote:
> Hi Benjamin,
> 
> As you have known, I'm working on improving drivers/staging/qlge. I'm
> not sure if I correctly understand some TODO items. Since you wrote the TODO
> list, could you explain some of the items or comment on the
> corresponding fix for me?
> 
> > * while in that area, using two 8k buffers to store one 9k frame is a poor
> >   choice of buffer size.
> 
> Currently, LARGE_BUFFER_MAX_SIZE is defined as 8192. How about we simply
> changing LARGE_BUFFER_MAX_SIZE to 4096? This is what
> drivers/net/ethernet/intel/e1000 does for jumbo frame right now.

I think that frags of 4096 would be better for allocations than the
current scheme. However, I don't know if simply changing that define is
the only thing to do.

BTW, e1000 was written long ago and not updated much, so it's not the
reference I would look at generally. Sadly I don't do much kernel
development anymore so I don't know which one to recommend either :/ If
I had to guess, I'd say ixgbe is a device of a similar vintage whose
driver has seen a lot better work.

> 
> > * in the "chain of large buffers" case, the driver uses an skb allocated with
> >   head room but only puts data in the frags.
> 
> Do you suggest implementing the copybreak feature which exists for e1000 for
> this driver, i.e., allocing a sk_buff and coping the header buffer into it?

No. From the "chain of large buffers" quote, I think I was referring to:

\ qlge_refill_sb
	skb = __netdev_alloc_skb(qdev->ndev, SMALL_BUFFER_SIZE, gfp);

\ qlge_build_rx_skb
		[...]
		/*
		 * The data is in a chain of large buffers
		[...]
			skb_fill_page_desc(skb, i,
					   lbq_desc->p.pg_chunk.page,
					   lbq_desc->p.pg_chunk.offset, size);
		[...]
		__pskb_pull_tail(skb, hlen);

So out of SMALL_BUFFER_SIZE, only hlen is used. Since SMALL_BUFFER_SIZE
is only 256, I'm not sure now if this really has any impact. In fact it
seems in line with ex. what ixgbe does (IXGBE_RX_HDR_SIZE).

However, in the same area, there is also
			skb = netdev_alloc_skb(qdev->ndev, length);
			[...]
			skb_fill_page_desc(skb, 0, lbq_desc->p.pg_chunk.page,
					   lbq_desc->p.pg_chunk.offset,
					   length);

Why is the skb allocated with "length" size? Something like
	skb = napi_alloc_skb(&rx_ring->napi, SMALL_BUFFER_SIZE);
would be better I think. The head only needs enough space for the
subsequent hlen pull.

BTW, it looks like commit f8c047be5401 ("staging: qlge: use qlge_*
prefix to avoid namespace clashes with other qlogic drivers") missed
some structures like struct rx_ring. Defines like SMALL_BUFFER_SIZE
should also have a prefix.

> 
> > * fix weird line wrapping (all over, ex. the ql_set_routing_reg() calls in
> >   qlge_set_multicast_list()).
> 
> This issue of weird line wrapping is supposed to be all over. But I can
> only find the ql_set_routing_reg() calls in qlge_set_multicast_list have
> this problem,
> 
> 			if (qlge_set_routing_reg
> 			    (qdev, RT_IDX_PROMISCUOUS_SLOT, RT_IDX_VALID, 1)) {
> 
> I can't find other places where functions calls put square and arguments
> in the new line. Could you give more hints?

Here are other examples of what I would call weird line wrapping:

	status = qlge_validate_flash(qdev,
				     sizeof(struct flash_params_8000) /
				   sizeof(u16),
				   "8000");

	status = qlge_wait_reg_rdy(qdev,
				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);

[...]

I put that item towards the end of the TODO list because I think the
misshapen formatting and the ridiculous overuse of () in expressions
serve a useful purpose. They clearly point to the code that hasn't yet
been rewritten; they make it easy to know what code to audit. Because of
that, I strongly think it would be better to tackle the TODO list
(roughly) in order.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: About improving the qlge Ethernet driver by following drivers/staging/qlge/TODO

[Coiby Xu]

On Wed, May 05, 2021 at 05:59:46PM +0900, Benjamin Poirier wrote:
>On 2021-05-04 21:14 +0800, Coiby Xu wrote:
>> Hi Benjamin,
>>
>> As you have known, I'm working on improving drivers/staging/qlge. I'm
>> not sure if I correctly understand some TODO items. Since you wrote the TODO
>> list, could you explain some of the items or comment on the
>> corresponding fix for me?
>>
>> > * while in that area, using two 8k buffers to store one 9k frame is a poor
>> >   choice of buffer size.
>>
>> Currently, LARGE_BUFFER_MAX_SIZE is defined as 8192. How about we simply
>> changing LARGE_BUFFER_MAX_SIZE to 4096? This is what
>> drivers/net/ethernet/intel/e1000 does for jumbo frame right now.
>
>I think that frags of 4096 would be better for allocations than the
>current scheme. However, I don't know if simply changing that define is
>the only thing to do.
>
>BTW, e1000 was written long ago and not updated much, so it's not the
>reference I would look at generally. Sadly I don't do much kernel
>development anymore so I don't know which one to recommend either :/ If
>I had to guess, I'd say ixgbe is a device of a similar vintage whose
>driver has seen a lot better work.

Thanks! I think we can simply set it to 4096,
  - I did a basic test. There are two interfaces managed by this qlge
    driver. I started a HTTP server binded to one interface. And curl -I
    THE_OTHER_INTERFACE was fine.
  - ixgbe also set page order to 0 unless FCoE is 
     // drivers/net/ethernet/intel/ixgbe/ixgbe.h
     /*
      * FCoE requires that all Rx buffers be over 2200 bytes in length.  Since
      * this is twice the size of a half page we need to double the page order
      * for FCoE enabled Rx queues.
      */
     static inline unsigned int ixgbe_rx_bufsz(struct ixgbe_ring *ring)
     {
     	if (test_bit(__IXGBE_RX_3K_BUFFER, &ring->state))
     		return IXGBE_RXBUFFER_3K;
     #if (PAGE_SIZE < 8192)
     	if (ring_uses_build_skb(ring))
     		return IXGBE_MAX_2K_FRAME_BUILD_SKB;
     #endif
     	return IXGBE_RXBUFFER_2K;
     }
     
     static inline unsigned int ixgbe_rx_pg_order(struct ixgbe_ring *ring)
     {
     #if (PAGE_SIZE < 8192)
     	if (test_bit(__IXGBE_RX_3K_BUFFER, &ring->state))
     		return 1;
     #endif
     	return 0;
     }
   - e1000 does the same thing.
>
>>
>> > * in the "chain of large buffers" case, the driver uses an skb allocated with
>> >   head room but only puts data in the frags.
>>
>> Do you suggest implementing the copybreak feature which exists for e1000 for
>> this driver, i.e., allocing a sk_buff and coping the header buffer into it?
>
>No. From the "chain of large buffers" quote, I think I was referring to:
>
>\ qlge_refill_sb
>	skb = __netdev_alloc_skb(qdev->ndev, SMALL_BUFFER_SIZE, gfp);
>
>\ qlge_build_rx_skb
>		[...]
>		/*
>		 * The data is in a chain of large buffers
>		[...]
>			skb_fill_page_desc(skb, i,
>					   lbq_desc->p.pg_chunk.page,
>					   lbq_desc->p.pg_chunk.offset, size);
>		[...]
>		__pskb_pull_tail(skb, hlen);
>
>So out of SMALL_BUFFER_SIZE, only hlen is used. Since SMALL_BUFFER_SIZE
>is only 256, I'm not sure now if this really has any impact. In fact it
>seems in line with ex. what ixgbe does (IXGBE_RX_HDR_SIZE).

Thanks for the clarification. It seems we needn't to change this place.

>
>However, in the same area, there is also
>			skb = netdev_alloc_skb(qdev->ndev, length);
>			[...]
>			skb_fill_page_desc(skb, 0, lbq_desc->p.pg_chunk.page,
>					   lbq_desc->p.pg_chunk.offset,
>					   length);
>
>Why is the skb allocated with "length" size? Something like
>	skb = napi_alloc_skb(&rx_ring->napi, SMALL_BUFFER_SIZE);
>would be better I think. The head only needs enough space for the
>subsequent hlen pull.

Thanks for the explanation! I think this place needs to modified. I'll
try to figure out how to reach this part of code so I can make sure the
change wouldn't introduce an issue.

Btw, qlge_process_mac_rx_page also has this issue,

     static void qlge_process_mac_rx_page(struct qlge_adapter *qdev,
     				     struct rx_ring *rx_ring,
     				     struct qlge_ib_mac_iocb_rsp *ib_mac_rsp,
     				     u32 length, u16 vlan_id)
     {
     	struct net_device *ndev = qdev->ndev;
     	struct sk_buff *skb = NULL;
     	void *addr;
     	struct qlge_bq_desc *lbq_desc = qlge_get_curr_lchunk(qdev, rx_ring);
     	struct napi_struct *napi = &rx_ring->napi;
     	size_t hlen = ETH_HLEN;
         ...
     	skb = netdev_alloc_skb(ndev, length);
     	skb_put_data(skb, addr, hlen);
         ...
     	skb_fill_page_desc(skb, 0, lbq_desc->p.pg_chunk.page,
     			   lbq_desc->p.pg_chunk.offset + hlen, length - hlen);

The code path would include qlge_process_mac_rx_page by
$ ping -I enp94s0f0 -c 4 -s 8800 -M do 192.168.1.209
after enabling jumbo frame.  

>
>BTW, it looks like commit f8c047be5401 ("staging: qlge: use qlge_*
>prefix to avoid namespace clashes with other qlogic drivers") missed
>some structures like struct rx_ring. Defines like SMALL_BUFFER_SIZE
>should also have a prefix.

Thanks for the reminding! When renaming rx_ring to a completion queue,
I'll add a prefix. I'll also add a prefix to other structures. 
>
>>
>> > * fix weird line wrapping (all over, ex. the ql_set_routing_reg() calls in
>> >   qlge_set_multicast_list()).
>>
>> This issue of weird line wrapping is supposed to be all over. But I can
>> only find the ql_set_routing_reg() calls in qlge_set_multicast_list have
>> this problem,
>>
>> 			if (qlge_set_routing_reg
>> 			    (qdev, RT_IDX_PROMISCUOUS_SLOT, RT_IDX_VALID, 1)) {
>>
>> I can't find other places where functions calls put square and arguments
>> in the new line. Could you give more hints?
>
>Here are other examples of what I would call weird line wrapping:
>
>	status = qlge_validate_flash(qdev,
>				     sizeof(struct flash_params_8000) /
>				   sizeof(u16),
>				   "8000");

Oh, I also found this one but I think it more fits another TODO item,
i.e., "* fix weird indentation (all over, ex. the for loops in
qlge_get_stats())".

>
>	status = qlge_wait_reg_rdy(qdev,
>				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
>
>[...]

Do you mean we should change it as follows,


	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
				               XGMAC_ADDR_XME);

"V=" in vim could detect some indentation problems but not the line
wrapping issue. So I just scanned the code manually to find this issue. 
Do you know there is a tool that could check if the code fits the kernel
coding style?

>
>I put that item towards the end of the TODO list because I think the
>misshapen formatting and the ridiculous overuse of () in expressions
>serve a useful purpose. They clearly point to the code that hasn't yet
>been rewritten; they make it easy to know what code to audit. Because of
>that, I strongly think it would be better to tackle the TODO list
>(roughly) in order.

Thanks for this tip! I'll tackle the TODO list in order.

-- 
Best regards,
Coiby

Re: About improving the qlge Ethernet driver by following drivers/staging/qlge/TODO

[Benjamin Poirier]

On 2021-05-07 09:32 +0800, Coiby Xu wrote:
[...]
> > > > * fix weird line wrapping (all over, ex. the ql_set_routing_reg() calls in
> > > >   qlge_set_multicast_list()).
> > > 
> > > This issue of weird line wrapping is supposed to be all over. But I can
> > > only find the ql_set_routing_reg() calls in qlge_set_multicast_list have
> > > this problem,
> > > 
> > > 			if (qlge_set_routing_reg
> > > 			    (qdev, RT_IDX_PROMISCUOUS_SLOT, RT_IDX_VALID, 1)) {
> > > 
> > > I can't find other places where functions calls put square and arguments
> > > in the new line. Could you give more hints?
> > 
> > Here are other examples of what I would call weird line wrapping:
> > 
> > 	status = qlge_validate_flash(qdev,
> > 				     sizeof(struct flash_params_8000) /
> > 				   sizeof(u16),
> > 				   "8000");
> 
> Oh, I also found this one but I think it more fits another TODO item,
> i.e., "* fix weird indentation (all over, ex. the for loops in
> qlge_get_stats())".
> 
> > 
> > 	status = qlge_wait_reg_rdy(qdev,
> > 				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
> > 
> > [...]
> 
> Do you mean we should change it as follows,
> 
> 
> 	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
> 				               XGMAC_ADDR_XME);

	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
				   XGMAC_ADDR_XME);

> 
> "V=" in vim could detect some indentation problems but not the line
> wrapping issue. So I just scanned the code manually to find this issue. Do
> you know there is a tool that could check if the code fits the kernel
> coding style?

See Documentation/process/coding-style.rst section 9.

You can search online for info about how to configure vim for the kernel
coding style, ex:
https://stackoverflow.com/questions/33676829/vim-configuration-for-linux-kernel-development

Re: About improving the qlge Ethernet driver by following drivers/staging/qlge/TODO

[Coiby Xu]

On Fri, May 07, 2021 at 09:16:20PM +0900, Benjamin Poirier wrote:
>On 2021-05-07 09:32 +0800, Coiby Xu wrote:
>[...]
>> > > > * fix weird line wrapping (all over, ex. the ql_set_routing_reg() calls in
>> > > >   qlge_set_multicast_list()).
>> > >
>> > > This issue of weird line wrapping is supposed to be all over. But I can
>> > > only find the ql_set_routing_reg() calls in qlge_set_multicast_list have
>> > > this problem,
>> > >
>> > > 			if (qlge_set_routing_reg
>> > > 			    (qdev, RT_IDX_PROMISCUOUS_SLOT, RT_IDX_VALID, 1)) {
>> > >
>> > > I can't find other places where functions calls put square and arguments
>> > > in the new line. Could you give more hints?
>> >
>> > Here are other examples of what I would call weird line wrapping:
>> >
>> > 	status = qlge_validate_flash(qdev,
>> > 				     sizeof(struct flash_params_8000) /
>> > 				   sizeof(u16),
>> > 				   "8000");
>>
>> Oh, I also found this one but I think it more fits another TODO item,
>> i.e., "* fix weird indentation (all over, ex. the for loops in
>> qlge_get_stats())".
>>
>> >
>> > 	status = qlge_wait_reg_rdy(qdev,
>> > 				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
>> >
>> > [...]
>>
>> Do you mean we should change it as follows,
>>
>>
>> 	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
>> 				               XGMAC_ADDR_XME);
>
>	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
>				   XGMAC_ADDR_XME);

Thanks! This is what I meant, i.e. qdev is aligned with XGMAC_ADDR_XME. 
Somehow when replying in the editor, the format is different from view 
mode. So I made a mistake by manually adding spaces to XGMAC_ADDR_XME 
to make it appear aligned with qdev in the editor.
>
>>
>> "V=" in vim could detect some indentation problems but not the line
>> wrapping issue. So I just scanned the code manually to find this issue. Do
>> you know there is a tool that could check if the code fits the kernel
>> coding style?
>
>See Documentation/process/coding-style.rst section 9.

Great. clang-format is exactly what I need. It could detect the above 
qlge_wait_reg_rdy issue and even automatically fix the issue. I will run
clang-format after finishing other TODO items.

>
>You can search online for info about how to configure vim for the kernel
>coding style, ex:
>https://stackoverflow.com/questions/33676829/vim-configuration-for-linux-kernel-development

Thanks. I'll use https://github.com/vivien/vim-linux-coding-style:)

-- 
Best regards,
Coiby

Re: About improving the qlge Ethernet driver by following drivers/staging/qlge/TODO

[Coiby Xu]

On Fri, May 07, 2021 at 09:32:39AM +0800, Coiby Xu wrote:
>On Wed, May 05, 2021 at 05:59:46PM +0900, Benjamin Poirier wrote:
>>On 2021-05-04 21:14 +0800, Coiby Xu wrote:
>>>Hi Benjamin,
>>>
>>>As you have known, I'm working on improving drivers/staging/qlge. I'm
>>>not sure if I correctly understand some TODO items. Since you wrote the TODO
>>>list, could you explain some of the items or comment on the
>>>corresponding fix for me?
>>>
[...]
>>
>>However, in the same area, there is also
>>			skb = netdev_alloc_skb(qdev->ndev, length);
>>			[...]
>>			skb_fill_page_desc(skb, 0, lbq_desc->p.pg_chunk.page,
>>					   lbq_desc->p.pg_chunk.offset,
>>					   length);
>>
>>Why is the skb allocated with "length" size? Something like
>>	skb = napi_alloc_skb(&rx_ring->napi, SMALL_BUFFER_SIZE);
>>would be better I think. The head only needs enough space for the
>>subsequent hlen pull.
>
>Thanks for the explanation! I think this place needs to modified. I'll
>try to figure out how to reach this part of code so I can make sure the
>change wouldn't introduce an issue.

After failing to reach to this part of code, it occurred to me this
may be what the first TODO item meant by "dead code" that handle
non-split case,

> * commit 7c734359d350 ("qlge: Size RX buffers based on MTU.", v2.6.33-rc1)
>   introduced dead code in the receive routines, which should be rewritten
>   anyways by the admission of the author himself, see the comment above
>   ql_build_rx_skb(). That function is now used exclusively to handle packets
>   that underwent header splitting but it still contains code to handle non
>   split cases.

Do you think so? Btw, I think you meant commit 4f848c0a9c265cb3457fbf842dbffd28e82a44fd
("qlge: Add RX frame handlers for non-split frames") here. Because it was in this
commit where the ql_process_mac_split_rx_intr was first introduced,  

     -static void ql_process_mac_rx_intr(struct ql_adapter *qdev,
     +static void ql_process_mac_split_rx_intr(struct ql_adapter *qdev,
                                        struct rx_ring *rx_ring,
     -                                  struct ib_mac_iocb_rsp *ib_mac_rsp)
     +                                  struct ib_mac_iocb_rsp *ib_mac_rsp,
     +                                  u16 vlan_id)


Another TODO item I don't understand is as follows,
> * the driver has a habit of using runtime checks where compile time checks are
>  possible (ex. ql_free_rx_buffers(), ql_alloc_rx_buffers())

Could be more specific about which runtime checks are used in ql_free_rx_buffers 
and ql_alloc_rx_buffers?

-- 
Best regards,
Coiby

Re: About improving the qlge Ethernet driver by following drivers/staging/qlge/TODO

[Benjamin Poirier]

On 2021-05-09 07:27 +0800, Coiby Xu wrote:
> On Fri, May 07, 2021 at 09:32:39AM +0800, Coiby Xu wrote:
> > On Wed, May 05, 2021 at 05:59:46PM +0900, Benjamin Poirier wrote:
> > > On 2021-05-04 21:14 +0800, Coiby Xu wrote:
> > > > Hi Benjamin,
> > > > 
> > > > As you have known, I'm working on improving drivers/staging/qlge. I'm
> > > > not sure if I correctly understand some TODO items. Since you wrote the TODO
> > > > list, could you explain some of the items or comment on the
> > > > corresponding fix for me?
> > > > 
> [...]
> > > 
> > > However, in the same area, there is also
> > > 			skb = netdev_alloc_skb(qdev->ndev, length);
> > > 			[...]
> > > 			skb_fill_page_desc(skb, 0, lbq_desc->p.pg_chunk.page,
> > > 					   lbq_desc->p.pg_chunk.offset,
> > > 					   length);
> > > 
> > > Why is the skb allocated with "length" size? Something like
> > > 	skb = napi_alloc_skb(&rx_ring->napi, SMALL_BUFFER_SIZE);
> > > would be better I think. The head only needs enough space for the
> > > subsequent hlen pull.
> > 
> > Thanks for the explanation! I think this place needs to modified. I'll
> > try to figure out how to reach this part of code so I can make sure the
> > change wouldn't introduce an issue.
> 
> After failing to reach to this part of code, it occurred to me this
> may be what the first TODO item meant by "dead code" that handle
> non-split case,
> 
> > * commit 7c734359d350 ("qlge: Size RX buffers based on MTU.", v2.6.33-rc1)
> >   introduced dead code in the receive routines, which should be rewritten
> >   anyways by the admission of the author himself, see the comment above
> >   ql_build_rx_skb(). That function is now used exclusively to handle packets
> >   that underwent header splitting but it still contains code to handle non
> >   split cases.
> 
> Do you think so? 

Yes

> Btw, I think you meant commit 4f848c0a9c265cb3457fbf842dbffd28e82a44fd
> ("qlge: Add RX frame handlers for non-split frames") here. Because it was in this
> commit where the ql_process_mac_split_rx_intr was first introduced,
> 
>     -static void ql_process_mac_rx_intr(struct ql_adapter *qdev,
>     +static void ql_process_mac_split_rx_intr(struct ql_adapter *qdev,
>                                        struct rx_ring *rx_ring,
>     -                                  struct ib_mac_iocb_rsp *ib_mac_rsp)
>     +                                  struct ib_mac_iocb_rsp *ib_mac_rsp,
>     +                                  u16 vlan_id)

It's possible that I referenced the wrong commit in the TODO. Clearly
there is dead code after commit 4f848c0a9c26 ("qlge: Add RX frame
handlers for non-split frames.") like you say. I don't remember for sure
if I had found some before even before that.

> 
> Another TODO item I don't understand is as follows,
> > * the driver has a habit of using runtime checks where compile time checks are
> >  possible (ex. ql_free_rx_buffers(), ql_alloc_rx_buffers())
> 
> Could be more specific about which runtime checks are used in
> ql_free_rx_buffers and ql_alloc_rx_buffers?

This specific example was fixed in commit
e4c911a73c89 ("staging: qlge: Remove rx_ring.type")

I forgot to update the TODO when making that commit.

Here are other examples:
a68a5b2fd3a2 ("staging: qlge: Remove bq_desc.maplen")
16714d98bf63 ("staging: qlge: Remove rx_ring.sbq_buf_size")
ec705b983b46 ("staging: qlge: Remove qlge_bq.len & size")

I don't remember of remaining examples to point you to. Maybe there
aren't but given that there were indeed quite a few, I would suggest
that you look at those commits and keep this item in mind as you work on
the other items earlier in the list. If at the end you think that this
is no longer a problem, then remove it from the list.

Re: About improving the qlge Ethernet driver by following drivers/staging/qlge/TODO

[Coiby Xu]

On Sun, May 09, 2021 at 04:51:02PM +0900, Benjamin Poirier wrote:
>On 2021-05-09 07:27 +0800, Coiby Xu wrote:
>> On Fri, May 07, 2021 at 09:32:39AM +0800, Coiby Xu wrote:
>> > On Wed, May 05, 2021 at 05:59:46PM +0900, Benjamin Poirier wrote:
>> > > On 2021-05-04 21:14 +0800, Coiby Xu wrote:
>> > > > Hi Benjamin,
>> > > >
>> > > > As you have known, I'm working on improving drivers/staging/qlge. I'm
>> > > > not sure if I correctly understand some TODO items. Since you wrote the TODO
>> > > > list, could you explain some of the items or comment on the
>> > > > corresponding fix for me?
>> > > >
>> [...]
>> > >
>> > > However, in the same area, there is also
>> > > 			skb = netdev_alloc_skb(qdev->ndev, length);
>> > > 			[...]
>> > > 			skb_fill_page_desc(skb, 0, lbq_desc->p.pg_chunk.page,
>> > > 					   lbq_desc->p.pg_chunk.offset,
>> > > 					   length);
>> > >
>> > > Why is the skb allocated with "length" size? Something like
>> > > 	skb = napi_alloc_skb(&rx_ring->napi, SMALL_BUFFER_SIZE);
>> > > would be better I think. The head only needs enough space for the
>> > > subsequent hlen pull.
>> >
>> > Thanks for the explanation! I think this place needs to modified. I'll
>> > try to figure out how to reach this part of code so I can make sure the
>> > change wouldn't introduce an issue.
>>
>> After failing to reach to this part of code, it occurred to me this
>> may be what the first TODO item meant by "dead code" that handle
>> non-split case,
>>
>> > * commit 7c734359d350 ("qlge: Size RX buffers based on MTU.", v2.6.33-rc1)
>> >   introduced dead code in the receive routines, which should be rewritten
>> >   anyways by the admission of the author himself, see the comment above
>> >   ql_build_rx_skb(). That function is now used exclusively to handle packets
>> >   that underwent header splitting but it still contains code to handle non
>> >   split cases.
>>
>> Do you think so?
>
>Yes
>
>> Btw, I think you meant commit 4f848c0a9c265cb3457fbf842dbffd28e82a44fd
>> ("qlge: Add RX frame handlers for non-split frames") here. Because it was in this
>> commit where the ql_process_mac_split_rx_intr was first introduced,
>>
>>     -static void ql_process_mac_rx_intr(struct ql_adapter *qdev,
>>     +static void ql_process_mac_split_rx_intr(struct ql_adapter *qdev,
>>                                        struct rx_ring *rx_ring,
>>     -                                  struct ib_mac_iocb_rsp *ib_mac_rsp)
>>     +                                  struct ib_mac_iocb_rsp *ib_mac_rsp,
>>     +                                  u16 vlan_id)
>
>It's possible that I referenced the wrong commit in the TODO. Clearly
>there is dead code after commit 4f848c0a9c26 ("qlge: Add RX frame
>handlers for non-split frames.") like you say. I don't remember for sure
>if I had found some before even before that.

Thanks for confirming it:)

>
>>
>> Another TODO item I don't understand is as follows,
>> > * the driver has a habit of using runtime checks where compile time checks are
>> >  possible (ex. ql_free_rx_buffers(), ql_alloc_rx_buffers())
>>
>> Could be more specific about which runtime checks are used in
>> ql_free_rx_buffers and ql_alloc_rx_buffers?
>
>This specific example was fixed in commit
>e4c911a73c89 ("staging: qlge: Remove rx_ring.type")
>
>I forgot to update the TODO when making that commit.
>
>Here are other examples:
>a68a5b2fd3a2 ("staging: qlge: Remove bq_desc.maplen")
>16714d98bf63 ("staging: qlge: Remove rx_ring.sbq_buf_size")
>ec705b983b46 ("staging: qlge: Remove qlge_bq.len & size")

Thanks for giving these examples!

>
>I don't remember of remaining examples to point you to. Maybe there
>aren't but given that there were indeed quite a few, I would suggest
>that you look at those commits and keep this item in mind as you work on
>the other items earlier in the list. If at the end you think that this
>is no longer a problem, then remove it from the list.

OK. I'll keep this item in mind. Thanks for the reminding!

-- 
Best regards,
Coiby