Re: [PATCH] PCI: mvebu: Mark driver as BROKEN

Re: [PATCH] PCI: mvebu: Mark driver as BROKEN

[Bjorn Helgaas]

[+cc linux-arm-kernel, beginning of thread:
https://lore.kernel.org/r/20230114164125.1298-1-pali@kernel.org]

On Tue, Aug 08, 2023 at 09:26:05AM +0200, Pali Rohár wrote:
> On Friday 04 August 2023 12:35:13 Russell King (Oracle) wrote:
> ...

> > For example, I have an Atheros PCIe WiFi card in an Armada 388 Clearfog
> > platform, and this works fine.
> > 
> > Uwe has a SATA controller for a bunch of disks in an Armada 370 based
> > NAS platform that is connected to PCIe, and removing PCIe support
> > effectively makes his platform utterly useless.
> > 
> > Please revert this patch.
> 
> Please do not revert it, instead start fixing problems.

We know that like all the other drivers, the mvebu driver isn't
perfect.  But I don't think effectively removing the driver completely
helps anybody.  If people try to use it and notice problems, we have a
chance to try to fix them.

Or maybe I'm missing your point.  I think you're suggesting that we
keep pci-mvebu in the tree but unselectable because it depends on
CONFIG_BROKEN.  What would be the advantage of doing that?

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: mvebu: Mark driver as BROKEN

[Pali Rohár]

On Tuesday 08 August 2023 11:26:27 Bjorn Helgaas wrote:
> [+cc linux-arm-kernel, beginning of thread:
> https://lore.kernel.org/r/20230114164125.1298-1-pali@kernel.org]
> 
> On Tue, Aug 08, 2023 at 09:26:05AM +0200, Pali Rohár wrote:
> > On Friday 04 August 2023 12:35:13 Russell King (Oracle) wrote:
> > ...
> 
> > > For example, I have an Atheros PCIe WiFi card in an Armada 388 Clearfog
> > > platform, and this works fine.
> > > 
> > > Uwe has a SATA controller for a bunch of disks in an Armada 370 based
> > > NAS platform that is connected to PCIe, and removing PCIe support
> > > effectively makes his platform utterly useless.
> > > 
> > > Please revert this patch.
> > 
> > Please do not revert it, instead start fixing problems.
> 
> We know that like all the other drivers, the mvebu driver isn't
> perfect.  But I don't think effectively removing the driver completely
> helps anybody.  If people try to use it and notice problems, we have a
> chance to try to fix them.

I do not want to remove it. I was trying to find somebody who can start
caring about issues. In last year I was resending patches, some smaller
which could improve situation, but most of them were ignored or rejected.

So I'm here and waiting for alternatives, and I'm prepared to review
changes and patches for mvebu, which can improve driver support.

But I do not see anything. The only one who wrote something useful was
Uwe as he wanted to do some git bisect (which normally indicates issues
or also fixup/patch).

Also some times ago Greg wrote something like that (mainline) kernel is
place for unsupported and broken drivers. But mvebu is going in this
direction.

How can I otherwise point out to start doing something in this area?

Or are you unhappy with the fact that there is at least somebody (me)
who is willing to do patch review for this marvell stuff? You should
have said it to me earlier.

But as I'm reading now, that I should go away, maybe you should have to
find also new reviewer for driver. Good luck here as there was nobody
who even wanted to do anything in this area.

> Or maybe I'm missing your point.  I think you're suggesting that we
> keep pci-mvebu in the tree but unselectable because it depends on
> CONFIG_BROKEN.  What would be the advantage of doing that?
> 
> Bjorn

Well, all knows here that driver is in bad state. In past there were
regressions and no accepted fixes for it. (At that time I prepared fix,
but you did not like it and nobody else comes with other alternative
patch).

There area other options which can be done now, if there are only people
like Russel who are complaining but refusing to do absolutely nothing.
For example mark driver as experimental (there is some Kconfig symbol
for it). Or add a new menuconfig selectable symbol which appropriately
warn all distributions about problems and would be dependency for mvebu.
(if you do not like broken symbol).

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: mvebu: Mark driver as BROKEN

[Russell King (Oracle)]

On Tue, Aug 08, 2023 at 09:20:26PM +0200, Pali Rohár wrote:
> There area other options which can be done now, if there are only people
> like Russel who are complaining but refusing to do absolutely nothing.

Fucking hell, here we go with the accusations again.

And you can't even be bothered to spell my name correctly.

Let's start over at your first accusation in this thread, because this
says everything about what the problem here is:

"You have received more changes and fixes for last 2 years for these
issues and you have done **nothing**. You even not said anything.
So you are the last one who can complain here.

And I'm stopped communicating with people who do not want to communicate
with me. This is pretty normal situation and you should have think about
it. No?"

Let's go through my points one by one, maybe you'll then understand,
because right now you seem to be totally immune to any appreciation
of anyone's situation other than your own.

1. pci-mvebu works 100% fine for me.

2. I do not see any problems with the hardware I have. If I have no
   problems, then by definition it works fine. How can I test - for
   example, failure to bring up the PCIe link (which I believe some
   of your patches were trying to address) when I HAVE NO PROBLEM WITH
   THE PCIe LINK NOT COMING UP? FFS, take a moment to think about that.

3. I have not asked you to work on it.

4. You do not have the right to demand that I do anything with it.

5. You decided to pick up some patches that I had in my tree and
   merge them into the kernel.

6. You objected when I rebased my branches on top of what you had
   merged (which were modified versions of my patches). I wanted to
   keep my changes intact.

   (Again, you have no damn right in this whole damned world to
   complain about what I do in my own git tree.)

7. You decided on your own back to mark the driver BROKEN - and you
   did that in such a way to *ensure* that no one who would have
   been using it would notice until after the patch was merged. That
   is underhand tactics. You effectively admit to that in my above
   quotation, which effectively states that you _actively_ decided
   to exclude me from that because I didn't interact with you.

> For example mark driver as experimental (there is some Kconfig symbol
> for it). Or add a new menuconfig selectable symbol which appropriately
> warn all distributions about problems and would be dependency for mvebu.
> (if you do not like broken symbol).

So distributions end up crippling systems that they've stated that they
support? That is the side effect of your patch?

I welcome your resignation as maintainer of this driver - and at this
point I think that is the best course of action given everything that
has happened. Yes - you have now driven me to actively seek your
resignation.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: mvebu: Mark driver as BROKEN

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 4120 bytes --]

[Cc += linux-arm-kernel list]

On Fri, Aug 04, 2023 at 12:35:13PM +0100, Russell King (Oracle) wrote:
> So it seems this patch got applied, but it wasn't Cc'd to
> linux-arm-kernel or anyone else, so those of us with platforms never
> had a chance to comment on it.
> 
> *** This change causes a regression to working setups. ***
> 
> It appears that the *only* reason this patch was proposed is to stop a
> kernel developer receiving problem reports from a set of users, but
> completely ignores that there is another group of users where this works
> fine - and thus the addition of this patch causes working setups to
> regress.
> 
> Because one is being bothered with problem reports is not a reason to
> mark a driver broken - and especially not doing so in a way that those
> who may be affected don't get an opportunity to comment on the patch!
> Also, there is _zero_ information provided on what the reported problems
> actually are, so no one else can guess what these issues are.
> 
> However, given that there are working setups and this change causes
> those to regress, it needs to be reverted.
> 
> For example, I have an Atheros PCIe WiFi card in an Armada 388 Clearfog
> platform, and this works fine.
> 
> Uwe has a SATA controller for a bunch of disks in an Armada 370 based
> NAS platform that is connected to PCIe, and removing PCIe support
> effectively makes his platform utterly useless.

While this is true there is really a problem on my platform with
accessing the hard disks via that pci controller and a 88SE9215 SATA
controller. While it seems to work in principle, it's incredible slow.

I intend to bisect that, 6.1.x is still fine. Don't know when I find the
time though, as there are a few things that are more important
currently.

+1 on some information about what is already known about the breakage.

> Please revert this patch.
> 
> Thanks.
> 
> On Sat, Jan 14, 2023 at 05:41:25PM +0100, Pali Rohár wrote:
> > People are reporting that pci-mvebu.c driver does not work with recent
> > mainline kernel. There are more bugs which prevents its for daily usage.
> > So lets mark it as broken for now, until somebody would be able to fix it
> > in mainline kernel.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > 
> > ---
> > Bjorn: I would really appreciate if you take this change and send it in
> > pull request for v6.2 release. There is no reason to wait more longer.
> > 
> > 
> > I'm periodically receiving emails that driver does not work correctly
> > anymore, PCIe cards are not detected or that they crashes during boot.
> > 
> > Some of the issues are handled in patches which are waiting on the list for
> > a long time and nobody cares for them. Some others needs investigation.
> > 
> > I'm really tired in replying to those user emails as I cannot do more in
> > this area. I have asked more people for help but either there were only
> > promises without any action for more than year or simple no direction how
> > to move forward or what to do with it.
> > 
> > So mark this driver as broken. Users would see the real current state
> > and hopefully will stop reporting me old or new bugs.
> > ---
> >  drivers/pci/controller/Kconfig | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> > index 1569d9a3ada0..b4a4d84a358b 100644
> > --- a/drivers/pci/controller/Kconfig
> > +++ b/drivers/pci/controller/Kconfig
> > @@ -9,6 +9,7 @@ config PCI_MVEBU
> >  	depends on MVEBU_MBUS
> >  	depends on ARM
> >  	depends on OF
> > +	depends on BROKEN
> >  	select PCI_BRIDGE_EMUL
> >  	help
> >  	 Add support for Marvell EBU PCIe controller. This PCIe controller
> > -- 
> > 2.20.1
> > 
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: mvebu: Mark driver as BROKEN

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2520 bytes --]

Hello,

On Fri, Aug 04, 2023 at 03:46:22PM +0200, Uwe Kleine-König wrote:
> On Fri, Aug 04, 2023 at 12:35:13PM +0100, Russell King (Oracle) wrote:
> > So it seems this patch got applied, but it wasn't Cc'd to
> > linux-arm-kernel or anyone else, so those of us with platforms never
> > had a chance to comment on it.
> > 
> > *** This change causes a regression to working setups. ***
> > 
> > It appears that the *only* reason this patch was proposed is to stop a
> > kernel developer receiving problem reports from a set of users, but
> > completely ignores that there is another group of users where this works
> > fine - and thus the addition of this patch causes working setups to
> > regress.
> > 
> > Because one is being bothered with problem reports is not a reason to
> > mark a driver broken - and especially not doing so in a way that those
> > who may be affected don't get an opportunity to comment on the patch!
> > Also, there is _zero_ information provided on what the reported problems
> > actually are, so no one else can guess what these issues are.
> > 
> > However, given that there are working setups and this change causes
> > those to regress, it needs to be reverted.
> > 
> > For example, I have an Atheros PCIe WiFi card in an Armada 388 Clearfog
> > platform, and this works fine.
> > 
> > Uwe has a SATA controller for a bunch of disks in an Armada 370 based
> > NAS platform that is connected to PCIe, and removing PCIe support
> > effectively makes his platform utterly useless.
> 
> While this is true there is really a problem on my platform with
> accessing the hard disks via that pci controller and a 88SE9215 SATA
> controller. While it seems to work in principle, it's incredible slow.
> 
> I intend to bisect that, 6.1.x is still fine. Don't know when I find the
> time though, as there are a few things that are more important
> currently.

I did that and found next-20230803 to be bad but next-20230804 is good.
I didn't debug that further and didn't spot anything obvious in

	git log --oneline --no-merges --left-right next-20230803...next-20230804

. I will just assume the problem is gone for good.

So now I'm in the position to say: For me PCI_MVEBU works fine and so I
support Russell's request to revert
b3574f579ece24439c90e9a179742c61205fbcfa.

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: mvebu: Mark driver as BROKEN

[Pali Rohár]

On Friday 04 August 2023 15:46:22 Uwe Kleine-König wrote:
> [Cc += linux-arm-kernel list]
> 
> On Fri, Aug 04, 2023 at 12:35:13PM +0100, Russell King (Oracle) wrote:
> > So it seems this patch got applied, but it wasn't Cc'd to
> > linux-arm-kernel or anyone else, so those of us with platforms never
> > had a chance to comment on it.
> > 
> > *** This change causes a regression to working setups. ***
> > 
> > It appears that the *only* reason this patch was proposed is to stop a
> > kernel developer receiving problem reports from a set of users, but
> > completely ignores that there is another group of users where this works
> > fine - and thus the addition of this patch causes working setups to
> > regress.
> > 
> > Because one is being bothered with problem reports is not a reason to
> > mark a driver broken - and especially not doing so in a way that those
> > who may be affected don't get an opportunity to comment on the patch!
> > Also, there is _zero_ information provided on what the reported problems
> > actually are, so no one else can guess what these issues are.
> > 
> > However, given that there are working setups and this change causes
> > those to regress, it needs to be reverted.
> > 
> > For example, I have an Atheros PCIe WiFi card in an Armada 388 Clearfog
> > platform, and this works fine.
> > 
> > Uwe has a SATA controller for a bunch of disks in an Armada 370 based
> > NAS platform that is connected to PCIe, and removing PCIe support
> > effectively makes his platform utterly useless.
> 
> While this is true there is really a problem on my platform with
> accessing the hard disks via that pci controller and a 88SE9215 SATA
> controller. While it seems to work in principle, it's incredible slow.

Exactly those are things which randomly does not work.

> I intend to bisect that, 6.1.x is still fine. Don't know when I find the
> time though, as there are a few things that are more important
> currently.
> 
> +1 on some information about what is already known about the breakage.
> 
> > Please revert this patch.
> > 
> > Thanks.
> > 
> > On Sat, Jan 14, 2023 at 05:41:25PM +0100, Pali Rohár wrote:
> > > People are reporting that pci-mvebu.c driver does not work with recent
> > > mainline kernel. There are more bugs which prevents its for daily usage.
> > > So lets mark it as broken for now, until somebody would be able to fix it
> > > in mainline kernel.
> > > 
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > 
> > > ---
> > > Bjorn: I would really appreciate if you take this change and send it in
> > > pull request for v6.2 release. There is no reason to wait more longer.
> > > 
> > > 
> > > I'm periodically receiving emails that driver does not work correctly
> > > anymore, PCIe cards are not detected or that they crashes during boot.
> > > 
> > > Some of the issues are handled in patches which are waiting on the list for
> > > a long time and nobody cares for them. Some others needs investigation.
> > > 
> > > I'm really tired in replying to those user emails as I cannot do more in
> > > this area. I have asked more people for help but either there were only
> > > promises without any action for more than year or simple no direction how
> > > to move forward or what to do with it.
> > > 
> > > So mark this driver as broken. Users would see the real current state
> > > and hopefully will stop reporting me old or new bugs.
> > > ---
> > >  drivers/pci/controller/Kconfig | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> > > index 1569d9a3ada0..b4a4d84a358b 100644
> > > --- a/drivers/pci/controller/Kconfig
> > > +++ b/drivers/pci/controller/Kconfig
> > > @@ -9,6 +9,7 @@ config PCI_MVEBU
> > >  	depends on MVEBU_MBUS
> > >  	depends on ARM
> > >  	depends on OF
> > > +	depends on BROKEN
> > >  	select PCI_BRIDGE_EMUL
> > >  	help
> > >  	 Add support for Marvell EBU PCIe controller. This PCIe controller
> > > -- 
> > > 2.20.1
> > > 
> > 
> > -- 
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> > 
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: mvebu: Mark driver as BROKEN

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1171 bytes --]

Hello Pali,

On Tue, Aug 08, 2023 at 09:27:01AM +0200, Pali Rohár wrote:
> On Friday 04 August 2023 15:46:22 Uwe Kleine-König wrote:
> > On Fri, Aug 04, 2023 at 12:35:13PM +0100, Russell King (Oracle) wrote:
> > > Uwe has a SATA controller for a bunch of disks in an Armada 370 based
> > > NAS platform that is connected to PCIe, and removing PCIe support
> > > effectively makes his platform utterly useless.
> > 
> > While this is true there is really a problem on my platform with
> > accessing the hard disks via that pci controller and a 88SE9215 SATA
> > controller. While it seems to work in principle, it's incredible slow.
> 
> Exactly those are things which randomly does not work.

I had this slow behaviour consistently on next-20230803 and
next-20230804 was fine. I thought that meant that there was something
fixed between these two trees. Do you suggest this is worth to
investigate as it might just be some butterfly effect that made the
problem go away?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: mvebu: Mark driver as BROKEN

[Pali Rohár]

On Tuesday 08 August 2023 09:38:22 Uwe Kleine-König wrote:
> Hello Pali,
> 
> On Tue, Aug 08, 2023 at 09:27:01AM +0200, Pali Rohár wrote:
> > On Friday 04 August 2023 15:46:22 Uwe Kleine-König wrote:
> > > On Fri, Aug 04, 2023 at 12:35:13PM +0100, Russell King (Oracle) wrote:
> > > > Uwe has a SATA controller for a bunch of disks in an Armada 370 based
> > > > NAS platform that is connected to PCIe, and removing PCIe support
> > > > effectively makes his platform utterly useless.
> > > 
> > > While this is true there is really a problem on my platform with
> > > accessing the hard disks via that pci controller and a 88SE9215 SATA
> > > controller. While it seems to work in principle, it's incredible slow.
> > 
> > Exactly those are things which randomly does not work.
> 
> I had this slow behaviour consistently on next-20230803 and
> next-20230804 was fine. I thought that meant that there was something
> fixed between these two trees. Do you suggest this is worth to
> investigate as it might just be some butterfly effect that made the
> problem go away?

These issues are there for a longer time, it started appearing after
5.15 lts version. And by your description it means that they were not
fixed yet.

> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel