linux-kernel - Re: [PATCH v7 04/13] slimbus: core: Add slim controllers support

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: <20171116164233.GB3187@localhost>
Date:   Thu, 16 Nov 2017 22:12:33 +0530
From:   Vinod Koul <vinod.koul@...el.com>
To:     srinivas.kandagatla@...aro.org
Cc:     gregkh@...uxfoundation.org, broonie@...nel.org,
        alsa-devel@...a-project.org, sdharia@...eaurora.org, bp@...e.de,
        poeschel@...onage.de, treding@...dia.com, andreas.noever@...il.com,
        alan@...ux.intel.com, mathieu.poirier@...aro.org, daniel@...ll.ch,
        jkosina@...e.cz, sharon.dvir1@...l.huji.ac.il, joe@...ches.com,
        davem@...emloft.net, james.hogan@...tec.com,
        michael.opdenacker@...e-electrons.com, robh+dt@...nel.org,
        pawel.moll@....com, mark.rutland@....com,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, arnd@...db.de
Subject: Re: [PATCH v7 04/13] slimbus: core: Add slim controllers support

On Wed, Nov 15, 2017 at 02:10:34PM +0000, srinivas.kandagatla@...aro.org wrote:

> +static void slim_dev_release(struct device *dev)
> +{
> +	struct slim_device *sbdev = to_slim_device(dev);
> +
> +	put_device(sbdev->ctrl->dev);

which device would that be?

> +static int slim_add_device(struct slim_controller *ctrl,
> +			   struct slim_device *sbdev,
> +			   struct device_node *node)
> +{
> +	sbdev->dev.bus = &slimbus_bus;
> +	sbdev->dev.parent = ctrl->dev;
> +	sbdev->dev.release = slim_dev_release;
> +	sbdev->dev.driver = NULL;
> +	sbdev->ctrl = ctrl;
> +
> +	dev_set_name(&sbdev->dev, "%x:%x:%x:%x",
> +				  sbdev->e_addr.manf_id,
> +				  sbdev->e_addr.prod_code,
> +				  sbdev->e_addr.dev_index,
> +				  sbdev->e_addr.instance);
> +
> +	get_device(ctrl->dev);

is this controller device and you ensuring it doesnt go away while you have
slaves on it?

> +static struct slim_device *slim_alloc_device(struct slim_controller *ctrl,
> +					     struct slim_eaddr *eaddr,
> +					     struct device_node *node)
> +{
> +	struct slim_device *sbdev;
> +	int ret;
> +
> +	sbdev = kzalloc(sizeof(struct slim_device), GFP_KERNEL);

Usual kernel way of doing is kzalloc(*sbdev)

> +void slim_report_absent(struct slim_device *sbdev)
> +{
> +	struct slim_controller *ctrl = sbdev->ctrl;
> +
> +	if (!ctrl)
> +		return;
> +
> +	/* invalidate logical addresses */
> +	mutex_lock(&ctrl->lock);
> +	sbdev->is_laddr_valid = false;
> +	mutex_unlock(&ctrl->lock);
> +
> +	ida_simple_remove(&ctrl->laddr_ida, sbdev->laddr);
> +	slim_device_update_status(sbdev, SLIM_DEVICE_STATUS_DOWN);
> +}
> +EXPORT_SYMBOL_GPL(slim_report_absent);

Do you have APIs for report present too, if so why not add te status in
argument as you may have common handling

> +static int slim_device_alloc_laddr(struct slim_device *sbdev,
> +				   u8 *laddr, bool report_present)
> +{
> +	struct slim_controller *ctrl = sbdev->ctrl;
> +	int ret;
> +
> +	mutex_lock(&ctrl->lock);
> +	if (ctrl->get_laddr) {
> +		ret = ctrl->get_laddr(ctrl, &sbdev->e_addr, laddr);
> +		if (ret < 0)
> +			goto err;
> +	} else if (report_present) {
> +		ret = ida_simple_get(&ctrl->laddr_ida,
> +				     0, SLIM_LA_MANAGER - 1, GFP_KERNEL);
> +		if (ret < 0)
> +			goto err;
> +
> +		*laddr = ret;
> +	} else {
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	if (ctrl->set_laddr) {
> +		ret = ctrl->set_laddr(ctrl, &sbdev->e_addr, *laddr);
> +		if (ret) {
> +			ret = -EINVAL;
> +			goto err;
> +		}
> +	}
> +
> +	sbdev->laddr = *laddr;

if you have this in sbdev, then why have this as an arg also?

> +	sbdev->is_laddr_valid = true;

shouldn't non-zero value signify that?

-- 
~Vinod