linux-kernel - Re: [PATCH 1/8] soundwire: bus_type: add master

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200305063646.GW4148@vkoul-mobl>
Date:   Thu, 5 Mar 2020 12:06:46 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
Cc:     alsa-devel@...a-project.org, tiwai@...e.de,
        gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>,
        Hui Wang <hui.wang@...onical.com>, broonie@...nel.org,
        srinivas.kandagatla@...aro.org, jank@...ence.com,
        slawomir.blauciak@...el.com, Sanyog Kale <sanyog.r.kale@...el.com>,
        Bard liao <yung-chuan.liao@...ux.intel.com>,
        Rander Wang <rander.wang@...ux.intel.com>
Subject: Re: [PATCH 1/8] soundwire: bus_type: add master_device/driver support

On 04-03-20, 09:17, Pierre-Louis Bossart wrote:
> 
> 
> > Were the above lines agreed or not? Do you see driver for master devices
> > or not? Greg was okay with as well as these patches but I am not okay
> > with the driver part for master, so I would like to see that removed.
> > 
> > Different reviewers can have different reasons.. I have given bunch of
> > reasons here, BUT I have not seen a single technical reason why this
> > cannot be done.
> 
> With all due respect, I consider Greg as THE reviewer for device/driver
> questions. Your earlier proposal to use platform devices was rejected by
> Greg, and we've lost an entire month in the process, so I am somewhat
> dubious on your proposal not to use a driver.
> 
> If you want a technical objection, let me restate what I already mentioned:
> 
> If you look at the hierarchy, we have
> 
> PCI device -> PCI driver
>   soundwire_master_device0
>      soundwire_slave(s) -> codec driver
>   ...
>   soundwire_master_deviceN
>      soundwire_slave(s) -> codec driver
> 
> You have not explained how I could possibly deal with power management
> without having a driver for the master_device(s). The pm_ops need to be
> inserted in a driver structure, which means we need a driver. And if we need
> a driver, then we might as well have a real driver with .probe .remove
> support, driver_register(), etc.

Please read the emails sent to you completely, including the reply on
2nd patch of this series. I think i am repeating this 3rd or 4th time
now.  Am going to repeat this info here to help move things.

Why do you need a extra driver for this. Do you have another set of
device object and driver for DSP code? But you do manage that, right?
I am proposing to simplify the device model here and have only one
device (SOF PCI) and driver (SOF PCI driver), which is created by actual
bus (PCI here) as you have in rest of the driver like HDA, DSP etc.

I have already recommended is to make the int-sdw a module which is
invoked by SOF PCI driver code (thereby all code uses SOF PCI device and
SOF PCI driver) directly. The DSP in my time for skl was a separate
module but used the parent objects.

The SOF sdw init (the place where sdw routines are invoked after DSP
load) can call sdw_probe and startup. Based on DSP sequencing you can
call these functions directly without waiting for extra device to be
probed etc.

I feel your flows will be greatly simplified as a result of this.

Second issue of PM:
 You do manage the DSP PM right? Similar way.
So here I would expect you to add functions/callbacks from SOF driver to
this module and call PM routines from SOF PM routines allowing you to
suspend/resume. Similar way DSP used to be managed.  Something like:
.sdw_suspend .sdw_resume functions/callbacks which will do sdw specific
pm configurations. You do not need module specific pm_ops, you can do
the required steps in callbacks from SOF driver

Bonus, this can be tuned and called at the specific places in DSP
suspend/resume flow, which is something I suspect you would want.

For places which need dev/driver objects like sdw dai's please pass the
SOF PCI dev object.

Is there any other technical reason left unexplored/unexplained?

> I really don't see what's broken or unnecessary with these patches.

Adding a layer for Intel in common code is unnecessary IMO. As
demonstrated above you can use the intel specific callback to do the
same task in intel specific way. I would very much prefer that approach
to solve this

We definitely need a sdw_master_device for everyone, but I don't believe
we need a sdw_master_device for Intel or anyone else.

> I would also kindly ask that you stop using exclamation marks and what I
> consider as hostile language. I've asked you multiple times, it's not
> professional, sorry.

oops, i would apologise for that. I seem to have a habit of using that
which indicates my surprise and not hostile language, maybe it is
cultural thing, but I would try to refrain. thanks for letting me know.

-- 
~Vinod