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

[Coiby Xu <coiby.xu@gmail.com>]

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