linux-kernel - Re: [PATCH] soundwire: intel: move to auxiliary bus

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: <aa9cfc49-913e-b3ba-1d92-2fab55bf4fa7@linux.intel.com>
Date:   Tue, 23 Mar 2021 12:29:48 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Greg KH <gregkh@...uxfoundation.org>, Vinod Koul <vkoul@...nel.org>
Cc:     alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        hui.wang@...onical.com, srinivas.kandagatla@...aro.org,
        sanyog.r.kale@...el.com,
        Bard Liao <yung-chuan.liao@...ux.intel.com>,
        rander.wang@...ux.intel.com, bard.liao@...el.com
Subject: Re: [PATCH] soundwire: intel: move to auxiliary bus

Thanks Greg and Vinod for the reviews

>>> -static int intel_master_probe(struct platform_device *pdev)
>>> +static int intel_link_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id)
>>>   {
>>> -	struct device *dev = &pdev->dev;
>>> +	struct device *dev = &auxdev->dev;
>>> +	struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev);
>>
>> Do we need another abstractions for resources here, why not aux dev
>> creation set the resources required and we skip this step...

Not sure what resources you are referring to?

this is just a container_of() and the documented way of extending the 
auxbus (see 
https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#example-usage)


struct sdw_intel_link_dev {
	struct auxiliary_device auxdev;
	struct sdw_intel_link_res link_res;
};

#define auxiliary_dev_to_sdw_intel_link_dev(auxiliary_dev) \
	container_of(auxiliary_dev, struct sdw_intel_link_dev, auxdev)

>>>   	struct sdw_intel *sdw;
>>>   	struct sdw_cdns *cdns;
>>>   	struct sdw_bus *bus;
>>> @@ -1346,14 +1347,14 @@ static int intel_master_probe(struct platform_device *pdev)
>>>   	cdns = &sdw->cdns;
>>>   	bus = &cdns->bus;
>>>   
>>> -	sdw->instance = pdev->id;
>>> -	sdw->link_res = dev_get_platdata(dev);
>>> +	sdw->instance = auxdev->id;
>>
>> so auxdev has id and still we pass id as argument :( Not sure if folks
>> can fix this now
> 
> That's odd, yeah, it should be fixed.

I think we are talking about different things?

this is defined in mod_devicetable.h:

struct auxiliary_device_id {
	char name[AUXILIARY_NAME_SIZE];
	kernel_ulong_t driver_data;
};

and used for the driver probe:

	ret = auxdrv->probe(auxdev, auxiliary_match_id(auxdrv->id_table, auxdev));

but there is a separate id:

struct auxiliary_device {
	struct device dev;
	const char *name;
	u32 id;
};

which is set during the device initialization in intel_init.c

	/* we don't use an IDA since we already have a link ID */
	auxdev->id = link_id;

In the auxiliary bus design, the parent has to take care of managing 
this id, be it with an IDA or as we do here with a physical link ID that 
is unique.

in short, I don't see how I could change the code given the differences 
in concepts?