Message-ID: <ZUmduQ_xKMHF6IY9@d3>
Date:   Mon, 6 Nov 2023 21:15:21 -0500
From:   Benjamin Poirier <benjamin.poirier@...il.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Kira <nyakov13@...il.com>, "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Jonathan Corbet <corbet@....net>,
        Manish Chopra <manishc@...vell.com>,
        GR-Linux-NIC-Dev@...vell.com, Coiby Xu <coiby.xu@...il.com>,
        "James E.J. Bottomley" <James.Bottomley@...senpartnership.com>,
        Helge Deller <deller@....de>, Sven Joachim <svenjoac@....de>,
        Ian Kent <raven@...maw.net>, netdev@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-parisc@...r.kernel.org, linux-staging@...ts.linux.dev
Subject: Re: [PATCH] staging: Revert "staging: qlge: Retire the driver"

On 2023-11-06 07:54 +0100, Greg Kroah-Hartman wrote:
> On Tue, Oct 31, 2023 at 02:04:00AM +1100, Benjamin Poirier wrote:
> > This reverts commit 875be090928d19ff4ae7cbaadb54707abb3befdf.
> > 
> > On All Hallows' Eve, fear and cower for it is the return of the undead
> > driver.
> > 
> > There was a report [1] from a user of a QLE8142 device. They would like for
> > the driver to remain in the kernel. Therefore, revert the removal of the
> > qlge driver.
> > 
> > [1] https://lore.kernel.org/netdev/566c0155-4f80-43ec-be2c-2d1ad631bf25@gmail.com/
> 
> <snip>
> 
> > --- /dev/null
> > +++ b/drivers/staging/qlge/TODO
> > @@ -0,0 +1,28 @@
> > +* 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
> > +  qlge_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.
> > +* truesize accounting is incorrect (ex: a 9000B frame has skb->truesize 10280
> > +  while containing two frags of order-1 allocations, ie. >16K)
> > +* while in that area, using two 8k buffers to store one 9k frame is a poor
> > +  choice of buffer size.
> > +* in the "chain of large buffers" case, the driver uses an skb allocated with
> > +  head room but only puts data in the frags.
> > +* rename "rx" queues to "completion" queues. Calling tx completion queues "rx
> > +  queues" is confusing.
> > +* struct rx_ring is used for rx and tx completions, with some members relevant
> > +  to one case only
> > +* the flow control implementation in firmware is buggy (sends a flood of pause
> > +  frames, resets the link, device and driver buffer queues become
> > +  desynchronized), disable it by default
> > +* the driver has a habit of using runtime checks where compile time checks are
> > +  possible (ex. qlge_free_rx_buffers())
> > +* reorder struct members to avoid holes if it doesn't impact performance
> > +* use better-suited apis (ex. use pci_iomap() instead of ioremap())
> > +* remove duplicate and useless comments
> > +* fix weird line wrapping (all over, ex. the qlge_set_routing_reg() calls in
> > +  qlge_set_multicast_list()).
> > +* remove useless casts (ex. memset((void *)mac_iocb_ptr, ...))
> > +* fix checkpatch issues
> 
> In looking at this again, are you sure you all want this in the tree?
> I'm glad to take the revert but ONLY if you are willing to then take a
> "move this to drivers/net/" patch for the code as well, WITH an actual
> maintainer and developer who is willing to do the work for this code.
> 
> In all the years that this has been in the staging tree, the listed
> maintainers have not been active at all from what I can remember, and
> obviously the above list of "things to fix" have not really been worked
> on at all.
> 
> So why should it be added back?  I understand there is at least one
> reported user, but for drivers in the staging tree, that's not a good
> reason to keep them around if there is not an actual maintainer that is
> willing to do the work.
> 
> Which reminds me, we should probably sweep the drivers/staging/ tree
> again to see what we can remove given a lack of real development.
> Normally we do that every other year or so, and this driver would fall
> into the "no one is doing anything with it" category and should be
> dropped.

Thank you for revisiting this topic. I agree with you that it's better
not to add orphaned code back into the kernel. I didn't want users to be
left out in the cold by the removal of the driver, so I just created a
dkms package as a fallback:
https://github.com/gobenji/qlge-dkms

People who want to use qlge with the latest kernel can use that package.
Since the driver code is not mainline quality and there isn't much
willingness to invest in its improvement, I think it's fitting that the
code lives out of tree. Of course, if somebody takes ownership of the
code and substantially improves it, they can submit it back to netdev.

Powered by blists - more mailing lists