Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 21 Jul 2022 22:46:07 +0200
From:   Pali =?utf-8?B?Um9ow6Fy?= <pali@kernel.org>
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Kishon Vijay Abraham I <kishon@ti.com>,
        Xiaowei Song <songxiaowei@hisilicon.com>,
        Binghui Wang <wangbinghui@hisilicon.com>,
        Thierry Reding <thierry.reding@gmail.com>,
        Ryder Lee <ryder.lee@mediatek.com>,
        Jianjun Wang <jianjun.wang@mediatek.com>,
        linux-pci@vger.kernel.org, Tom Joseph <tjoseph@cadence.com>,
        Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
        Ley Foon Tan <ley.foon.tan@intel.com>,
        linux-kernel@vger.kernel.org
Subject: Re: Why set .suppress_bind_attrs even though .remove() implemented?
Message-ID: <20220721204607.xklzyklbgwcgepjm@pali>
References: <20220721195433.GA1747571@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220721195433.GA1747571@bhelgaas>
User-Agent: NeoMutt/20180716
Precedence: bulk

Hello!

On Thursday 21 July 2022 14:54:33 Bjorn Helgaas wrote:
> The j721e, kirin, tegra, and mediatek drivers all implement .remove().
> 
> They also set ".suppress_bind_attrs = true".  I think this means
> bus_add_driver() will not create the "bind" and "unbind" sysfs
> attributes for the driver that would allow users to users to manually
> attach and detach devices from it.
> 
> Is there a reason for this, or should these drivers stop setting
> .suppress_bind_attrs?

I have already asked this question during review of kirin driver:
https://lore.kernel.org/linux-pci/20211031205527.ochhi72dfu4uidii@pali/

Microchip driver wanted to change its type from bool to tristate
https://lore.kernel.org/linux-pci/20220420093449.38054-1-u.kleine-koenig@pengutronix.de/t/#u
and after discussion it seems that it is needed to do more work for this
driver.

> For example, Pali and Ley Foon *did* stop setting .suppress_bind_attrs
> when adding .remove() methods in these commits:
> 
>   0746ae1be121 ("PCI: mvebu: Add support for compiling driver as module")
>   526a76991b7b ("PCI: aardvark: Implement driver 'remove' function and allow to build it as module")
>   ec15c4d0d5d2 ("PCI: altera: Allow building as module")
> 
> Bjorn

I added it for both pci-mvebu.c and pci-aardvark.c. And just few days
ago I realized why suppress_bind_attrs was set to true and remove method
was not implemented.

Implementing remove method is not really simple, specially when pci
controller driver implements also interrupt controller (e.g. for
handling legacy interrupts).

Here are waiting fixup patches for pci-mvebu.c and pci-aardvark.c which
fixes .remove callback. Without these patches calling 'rmmod driver' let
dangling pointer in kernel which may cause random kernel crashes. See:

https://lore.kernel.org/linux-pci/20220709161858.15031-1-pali@kernel.org/
https://lore.kernel.org/linux-pci/20220711120626.11492-1-pali@kernel.org/
https://lore.kernel.org/linux-pci/20220711120626.11492-2-pali@kernel.org/

So I would suggest to do more detailed review when adding .remove
callback for pci controller driver (or when remove suppress_bind_attrs)
and do more testings and checking if all IRQ mappings are disposed.