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

["Pali Rohár" <pali@kernel.org>]

On Friday 04 August 2023 12:06:55 Bjorn Helgaas wrote:
> [+cc Krzysztof]
> 
> 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.
> > 
> > Please revert this patch.
> 
> Sorry for the inconvenience.
> 
> I was under the mistaken impression that making the driver depend on
> CONFIG_BROKEN would keep the driver available but only if the user
> explicitly requested it, similar to how 
> CONFIG_COMPILE_TEST works.  But obviously that's not the case, so
> we'll revert the change.
> 
> I queued up the revert below, including a note in the Kconfig help
> text about the known issues.
> 
> commit 814b6bb15367 ("Revert "PCI: mvebu: Mark driver as BROKEN"")
> Author: Bjorn Helgaas <bhelgaas@google.com>
> Date:   Fri Aug 4 11:54:43 2023 -0500
> 
>     Revert "PCI: mvebu: Mark driver as BROKEN"
>     
>     b3574f579ece ("PCI: mvebu: Mark driver as BROKEN") made it impossible to
>     enable the pci-mvebu driver.  The driver does have known problems, but as
>     Russell and Uwe reported, it does work in some configurations, so removing
>     it broke some working setups.
>     
>     Revert b3574f579ece so pci-mvebu is available.  Mention the known problems
>     in the Kconfig help text.
>     
>     Reported-by: Russell King (Oracle) <linux@armlinux.org.uk>
>     Link: https://lore.kernel.org/r/ZMzicVQEyHyZzBOc@shell.armlinux.org.uk
>     Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>     Link: https://lore.kernel.org/r/20230804134622.pmbymxtzxj2yfhri@pengutronix.de
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 

What you are trying to achieve with this patch now? Do you think that it
is really correct to show that everything is working for everybody
correctly? Use a common sense here.

Or this is a way how kernel people are fixing bugs?

Now I'm starting understand why majority of HW industry say to not use
"unsupported mainline kernel" and instead use our prepared patched
kernels...

> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index 8d49bad7f847..478f158b2dfb 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -179,13 +179,15 @@ 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
>  	 is used on 32-bit Marvell ARM SoCs: Dove, Kirkwood, Armada 370,
>  	 Armada XP, Armada 375, Armada 38x and Armada 39x.
>  
> +	 This driver has known problems that may cause crashes during boot
> +	 and failure to detect PCIe devices in some cases.
> +
>  config PCIE_MEDIATEK
>  	tristate "MediaTek PCIe controller"
>  	depends on ARCH_AIROHA || ARCH_MEDIATEK || COMPILE_TEST