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  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]
Date:   Thu, 9 Nov 2017 21:14:07 +0000
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Vinod Koul <vinod.koul@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        ALSA <alsa-devel@...a-project.org>, Mark <broonie@...nel.org>,
        Takashi <tiwai@...e.de>,
        Pierre <pierre-louis.bossart@...ux.intel.com>,
        Sanyog Kale <sanyog.r.kale@...el.com>,
        Shreyas NC <shreyas.nc@...el.com>, patches.audio@...el.com,
        alan@...ux.intel.com,
        Charles Keepax <ckeepax@...nsource.cirrus.com>,
        Sagar Dharia <sdharia@...eaurora.org>, plai@...eaurora.org,
        Sudheer Papothi <spapothi@...eaurora.org>
Subject: Re: [PATCH 02/14] soundwire: Add SoundWire bus type



On 19/10/17 04:03, Vinod Koul wrote:
> This adds the base SoundWire bus type, bus and driver registration.
> along with changes to module device table for new SoundWire
> device type.
> 
> Signed-off-by: Sanyog Kale <sanyog.r.kale@...el.com>
> Signed-off-by: Vinod Koul <vinod.koul@...el.com>
> ---

> +++ b/drivers/soundwire/Kconfig
> @@ -0,0 +1,22 @@
> +#
> +# SoundWire subsystem configuration
> +#
> +
> +menuconfig SOUNDWIRE
> +	bool "SoundWire support"

Any reason why this subsystem can not be build as module?

> +	---help---
> +	  SoundWire is a 2-Pin interface with data and clock line ratified
> +	  by the MIPI Alliance. SoundWire is used for transporting data
> +	  typically related to audio functions. SoundWire interface is

> +#ifndef __SDW_BUS_H
> +#define __SDW_BUS_H
> +
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/acpi.h>
Do you need these headers here?

> +#include <linux/soundwire/sdw.h>
> +
> +int sdw_slave_modalias(struct sdw_slave *slave, char *buf, size_t size);
> +
> +#endif /* __SDW_BUS_H */
> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
> new file mode 100644
> index 000000000000..a14d1de80afa
> --- /dev/null
> +++ b/drivers/soundwire/bus_type.c
>
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/soundwire/sdw.h>
> +#include "bus.h"
> +
> +/**
> + * sdw_get_device_id: find the matching SoundWire device id
> + *
function name should end with () - according to kernel doc.

> + * @slave: SoundWire Slave device
> + * @drv: SoundWire Slave Driver
> + *
> + * The match is done by comparing the mfg_id and part_id from the
> + * struct sdw_device_id. class_id is unused, as it is a placeholder
> + * in MIPI Spec.
> + */

BTW, This is a static private function, why are we adding kernel doc for 
this?

> +static const struct sdw_device_id *
> +sdw_get_device_id(struct sdw_slave *slave, struct sdw_driver *drv)
> +{
> +	const struct sdw_device_id *id = drv->id_table;
> +
> +	while (id && id->mfg_id) {
> +		if (slave->id.mfg_id == id->mfg_id &&
> +				slave->id.part_id == id->part_id) {
> +			return id;
> +		}
> +		id++;
> +	}
> +
> +	return NULL;
> +}
> +
> +static int sdw_bus_match(struct device *dev, struct device_driver *ddrv)
> +{
> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +	struct sdw_driver *drv = drv_to_sdw_driver(ddrv);
> +
> +	return !!sdw_get_device_id(slave, drv);
> +}
> +
> +int sdw_slave_modalias(struct sdw_slave *slave, char *buf, size_t size)
> +{
> +	/* modalias is sdw:m<mfg_id>p<part_id> */
> +
> +	return snprintf(buf, size, "sdw:m%04Xp%04X\n",
> +			slave->id.mfg_id, slave->id.part_id);
> +}
> +
> +static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
> +{
> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +	char modalias[32];
> +
> +	sdw_slave_modalias(slave, modalias, sizeof(modalias));
> +
> +	if (add_uevent_var(env, "MODALIAS=%s", modalias))
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +struct bus_type sdw_bus_type = {
> +	.name = "soundwire",
> +	.match = sdw_bus_match,
> +	.uevent = sdw_uevent,
> +};
> +EXPORT_SYMBOL(sdw_bus_type);
> +
> +static int sdw_drv_probe(struct device *dev)
> +{
> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +	struct sdw_driver *drv = drv_to_sdw_driver(dev->driver);
> +	const struct sdw_device_id *id;
> +	int ret;
> +
> +	id = sdw_get_device_id(slave, drv);

By this time we must have already matched dev and driver by the ID, 
shouldn't it be just slave->id  here?
> +	if (!id)
> +		return -ENODEV;
> +
> +	/*
> +	 * attach to power domain but don't turn on (last arg)
> +	 */
> +	ret = dev_pm_domain_attach(dev, false);
> +	if (ret) {
Shouldn't it just handle the EPROBE_DEFER case and ignore it for other 
errors.


> +		dev_err(dev, "Failed to attach PM domain: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = drv->probe(slave, id);
> +	if (ret) {
> +		dev_err(dev, "Probe of %s failed: %d\n", drv->name, ret);
> +		return ret;
> +	}


What happens if the slave driver is built as module and loaded after the 
slave device is attached to the bus. How does the slave driver get 
updated status in this case?

We have similar usecase in slimbus too.

> +
> +	return 0;
> +}
> +




> 

Powered by blists - more mailing lists