Re: [PATCH] staging: Revert "staging: qlge: Retire the driver"

Re: [PATCH] staging: Revert "staging: qlge: Retire the driver"

[Greg Kroah-Hartman]

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/

Who's going to maintain this?

> Reported by: Kira <nyakov13@gmail.com>
> Signed-off-by: Benjamin Poirier <benjamin.poirier@gmail.com>
> ---
> 
> Notes:
>     Once the removal and revert show up in the net-next tree, I plan to send a
>     followup patch to move the driver to drivers/net/ as discussed earlier:
>     https://lore.kernel.org/netdev/20231019074237.7ef255d7@kernel.org/

are you going to be willing to maintain this and keep it alive?

I'm all this, if you want to, but I would like it out of staging.  So
how about applying this, and a follow-on one that moves it there once
-rc1 is out?  And it probably should be in the 'net' tree, as you don't
want 6.7 to come out without the driver at all, right?

> +QLOGIC QLGE 10Gb ETHERNET DRIVER
> +M:	Manish Chopra <manishc@marvell.com>
> +M:	GR-Linux-NIC-Dev@marvell.com
> +M:	Coiby Xu <coiby.xu@gmail.com>
> +L:	netdev@vger.kernel.org
> +S:	Supported
> +F:	Documentation/networking/device_drivers/qlogic/qlge.rst
> +F:	drivers/staging/qlge/

It's obvious taht these people are not maintaining this code, so they
should be dropped from the MAINTAINERS file as well.

thanks,

greg k-h

Re: [PATCH] staging: Revert "staging: qlge: Retire the driver"

[Benjamin Poirier]

On 2023-10-30 16:25 +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/
> 
> Who's going to maintain this?

I was planning to update the MAINTAINERS entry to
S:	Orphan
when moving it back to drivers/net/. Would you prefer that I do that
change in a second patch right after the revert in staging? That would
certainly make things clearer.

> > Reported by: Kira <nyakov13@gmail.com>
> > Signed-off-by: Benjamin Poirier <benjamin.poirier@gmail.com>
> > ---
> > 
> > Notes:
> >     Once the removal and revert show up in the net-next tree, I plan to send a
> >     followup patch to move the driver to drivers/net/ as discussed earlier:
> >     https://lore.kernel.org/netdev/20231019074237.7ef255d7@kernel.org/
> 
> are you going to be willing to maintain this and keep it alive?

No.

> I'm all this, if you want to, but I would like it out of staging.  So

I'd like it out of staging as well. Since nobody wants to maintain it, I
think it should be deleted. However, my understanding is that Jakub is
willing to take it back into drivers/net/ as-is given that there is at
least one user. Jakub, did I understand that correctly?

> how about applying this, and a follow-on one that moves it there once
> -rc1 is out?  And it probably should be in the 'net' tree, as you don't
> want 6.7 to come out without the driver at all, right?

Right about making sure 6.7 includes the driver. The 'net' tree is
usually for fixes hence why I would send to net-next. So the driver
would still be in staging for 6.7 (if you include the revert in your
6.7-rc1 submission) and would be back in drivers/net/ for 6.8.

> > +QLOGIC QLGE 10Gb ETHERNET DRIVER
> > +M:	Manish Chopra <manishc@marvell.com>
> > +M:	GR-Linux-NIC-Dev@marvell.com
> > +M:	Coiby Xu <coiby.xu@gmail.com>
> > +L:	netdev@vger.kernel.org
> > +S:	Supported
> > +F:	Documentation/networking/device_drivers/qlogic/qlge.rst
> > +F:	drivers/staging/qlge/
> 
> It's obvious taht these people are not maintaining this code, so they
> should be dropped from the MAINTAINERS file as well.

I agree.

Re: [PATCH] staging: Revert "staging: qlge: Retire the driver"

[Benjamin Poirier]

On 2023-10-30 12:33 -0400, Benjamin Poirier wrote:
> On 2023-10-30 16:25 +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/
> > 
> > Who's going to maintain this?
> 
> I was planning to update the MAINTAINERS entry to
> S:	Orphan
> when moving it back to drivers/net/. Would you prefer that I do that
> change in a second patch right after the revert in staging? That would
> certainly make things clearer.
> 
> > > Reported by: Kira <nyakov13@gmail.com>
> > > Signed-off-by: Benjamin Poirier <benjamin.poirier@gmail.com>
> > > ---
> > > 
> > > Notes:
> > >     Once the removal and revert show up in the net-next tree, I plan to send a
> > >     followup patch to move the driver to drivers/net/ as discussed earlier:
> > >     https://lore.kernel.org/netdev/20231019074237.7ef255d7@kernel.org/
> > 
> > are you going to be willing to maintain this and keep it alive?
> 
> No.
> 
> > I'm all this, if you want to, but I would like it out of staging.  So
> 
> I'd like it out of staging as well. Since nobody wants to maintain it, I
> think it should be deleted. However, my understanding is that Jakub is
> willing to take it back into drivers/net/ as-is given that there is at
> least one user. Jakub, did I understand that correctly?
> 
> > how about applying this, and a follow-on one that moves it there once
> > -rc1 is out?  And it probably should be in the 'net' tree, as you don't
> > want 6.7 to come out without the driver at all, right?
> 
> Right about making sure 6.7 includes the driver. The 'net' tree is
> usually for fixes hence why I would send to net-next. So the driver
> would still be in staging for 6.7 (if you include the revert in your
> 6.7-rc1 submission) and would be back in drivers/net/ for 6.8.

Rereading, I think I misunderstood your suggestion. You're suggesting
that I submit the revert through the net tree rather than the staging
tree. That sounds good. I'll do that once the removal shows up there.

Re: [PATCH] staging: Revert "staging: qlge: Retire the driver"

[Jakub Kicinski]

On Mon, 30 Oct 2023 12:33:55 -0400 Benjamin Poirier wrote:
> > I'm all this, if you want to, but I would like it out of staging.  So  
> 
> I'd like it out of staging as well. Since nobody wants to maintain it, I
> think it should be deleted. However, my understanding is that Jakub is
> willing to take it back into drivers/net/ as-is given that there is at
> least one user. Jakub, did I understand that correctly?

Yes, of all the bad options this one seems least.. unusual.

To be clear - we can take the revert to net, and then the follow up /
move would go to net-next (after -rc1, for v6.8)? My initial thought 
was to get the move done in 6.7 as well, but I guess that'd be churn 
we don't want at this stage..

Re: [PATCH] staging: Revert "staging: qlge: Retire the driver"

[Greg Kroah-Hartman]

On Mon, Oct 30, 2023 at 12:33:55PM -0400, Benjamin Poirier wrote:
> On 2023-10-30 16:25 +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/
> > 
> > Who's going to maintain this?
> 
> I was planning to update the MAINTAINERS entry to
> S:	Orphan
> when moving it back to drivers/net/. Would you prefer that I do that
> change in a second patch right after the revert in staging? That would
> certainly make things clearer.

I would prefer not having orphaned code in the kernel tree.  Again, who
is going to support this?  It was dropped because there is no owner and
the company doesn't care anymore.  We can't add it back if there is no
one who will do the real-work to fix it up and get it out of staging.
Just magically moving it there isn't going to be a solution either.

> > > Reported by: Kira <nyakov13@gmail.com>
> > > Signed-off-by: Benjamin Poirier <benjamin.poirier@gmail.com>
> > > ---
> > > 
> > > Notes:
> > >     Once the removal and revert show up in the net-next tree, I plan to send a
> > >     followup patch to move the driver to drivers/net/ as discussed earlier:
> > >     https://lore.kernel.org/netdev/20231019074237.7ef255d7@kernel.org/
> > 
> > are you going to be willing to maintain this and keep it alive?
> 
> No.
> 
> > I'm all this, if you want to, but I would like it out of staging.  So
> 
> I'd like it out of staging as well. Since nobody wants to maintain it, I
> think it should be deleted. However, my understanding is that Jakub is
> willing to take it back into drivers/net/ as-is given that there is at
> least one user. Jakub, did I understand that correctly?
> 
> > how about applying this, and a follow-on one that moves it there once
> > -rc1 is out?  And it probably should be in the 'net' tree, as you don't
> > want 6.7 to come out without the driver at all, right?
> 
> Right about making sure 6.7 includes the driver. The 'net' tree is
> usually for fixes hence why I would send to net-next. So the driver
> would still be in staging for 6.7 (if you include the revert in your
> 6.7-rc1 submission) and would be back in drivers/net/ for 6.8.

Let's wait until 6.7-rc1 is out and then, if the netdev developers want
to take this on, they can revert it and move it to drivers/net/.

But right now, my tree is frozen, it's the middle of the merge window,
let's wait 2 weeks please.

thanks,

greg k-h

Re: [PATCH] staging: Revert "staging: qlge: Retire the driver"

[Greg Kroah-Hartman]

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.

thanks,

greg k-h

Re: [PATCH] staging: Revert "staging: qlge: Retire the driver"

[Benjamin Poirier]

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.