Message-ID: <YLS4N2KgzfsMBD1c@vkoul-mobl.Dlink>
Date:   Mon, 31 May 2021 15:49:35 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
Cc:     Bard Liao <yung-chuan.liao@...ux.intel.com>,
        alsa-devel@...a-project.org, gregkh@...uxfoundation.org,
        linux-kernel@...r.kernel.org, hui.wang@...onical.com,
        sanyog.r.kale@...el.com, rander.wang@...ux.intel.com,
        bard.liao@...el.com, Leon Romanovsky <leon@...nel.org>,
        Jason Gunthorpe <jgg@...dia.com>,
        Dave Ertman <david.m.ertman@...el.com>,
        Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>
Subject: Re: [PATCH v4] soundwire: intel: move to auxiliary bus

On 25-05-21, 13:30, Pierre-Louis Bossart wrote:
> On 5/11/21 12:21 AM, Bard Liao wrote:
> > From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
> > 
> > Now that the auxiliary_bus exists, there's no reason to use platform
> > devices as children of a PCI device any longer.
> > 
> > This patch refactors the code by extending a basic auxiliary device
> > with Intel link-specific structures that need to be passed between
> > controller and link levels. This refactoring is much cleaner with no
> > need for cross-pointers between device and link structures.
> > 
> > Note that the auxiliary bus API has separate init and add steps, which
> > requires more attention in the error unwinding paths. The main loop
> > needs to deal with kfree() and auxiliary_device_uninit() for the
> > current iteration before jumping to the common label which releases
> > everything allocated in prior iterations.
> > 
> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
> > Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@...ux.intel.com>
> > Reviewed-by: Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>
> > Signed-off-by: Bard Liao <yung-chuan.liao@...ux.intel.com>
> > ---
> > v2:
> >   - add link_dev_register for all kzalloc, device_init, and device_add.
> > v3:
> >   - Modify the function description of sdw_intel_probe() which was
> >     missing in previous version.
> > v4:
> >   - Move intel_link_dev_unregister(ldev) before pm_runtime_put_noidle(
> >     ldev->link_res.dev) to fix use-after-free reported by KASAN.
> 
> Two years ago, GregKH stated in
> https://lore.kernel.org/lkml/20190509181812.GA10776@kroah.com/
> 
> "So soundwire is creating platform devices?  Ick, no!  Don't do that"
> 
> Fast forward two years, this patch provides a solution to remove the use of
> the platform devices with the auxiliary bus. This move does not add any new
> functionality, it's just a replacement of one type of device by another.
> 
> The reviews have been rather limited since the first version shared on March
> 22.
> 
> a) I updated the code to follow the model of the Mellanox driver in
> 
> https://elixir.bootlin.com/linux/latest/source/include/linux/mlx5/driver.h#L545
> 
> I hope this addresses GregKH's feedback on the need for a 'register'
> function to combined the two init and add steps. I didn't see a path to add
> a generic register function in the auxiliary bus code since between init and
> add there is a need to setup domain-specific structures. Other contributors
> to the auxiliary bus (CC:ed) also didn't have a burning desire to add this
> capability.
> 
> b) Vinod commented:
> 
> "What I would like to see the end result is that sdw driver for Intel
> controller here is a simple auxdev device and no additional custom setup
> layer required... which implies that this handling should be moved into
> auxdev or Intel code setting up auxdev..."
> 
> I was unable to figure out what this comment hinted at: the auxbus is
> already handled in the intel_init.c and intel.c files and the auxbus is used
> to model a set of links/managers below the PCI device, not the controller
> itself. There is also no such thing as a simple auxdev device used in the
> kernel today, the base layer is meant to be extended with domain-specific
> structures. There is really no point in creating a simple auxbus device
> without extensions.

<back from vacations>

I would like to see that the init_init.c removed completely, that is my
ask here

This layer was created by me to aid in creating the platform devices.
Also the mistake was not to use platform resources and instead pass a
custom structure for resources (device iomem address, irq etc)

I would like to see is the PCI/SOF parent driver create the sdw aux
device and that should be all needed to be done. The aux device would be
probed by sdw driver. No custom resource structs for resources please.

If that is not possible, I would like to understand technical details of
why that would be that case. If required necessary changes should be
made to aux bus to handle and not have sequencing issue which you had
trouble with platform approach.

Thanks
-- 
~Vinod

Powered by blists - more mailing lists