linux-kernel - Re: [alsa-devel] [PATCH 06/14] soundwire: Add IO transfer

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]

Date:   Thu, 19 Oct 2017 11:13:48 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Vinod Koul <vinod.koul@...el.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        ALSA <alsa-devel@...a-project.org>,
        Charles Keepax <ckeepax@...nsource.cirrus.com>,
        Sudheer Papothi <spapothi@...eaurora.org>, plai@...eaurora.org,
        LKML <linux-kernel@...r.kernel.org>,
        Pierre <pierre-louis.bossart@...ux.intel.com>,
        patches.audio@...el.com, Mark <broonie@...nel.org>,
        srinivas.kandagatla@...aro.org, Shreyas NC <shreyas.nc@...el.com>,
        Sanyog Kale <sanyog.r.kale@...el.com>,
        Sagar Dharia <sdharia@...eaurora.org>, alan@...ux.intel.com
Subject: Re: [alsa-devel] [PATCH 06/14] soundwire: Add IO transfer

On Thu, 19 Oct 2017 05:03:22 +0200,
Vinod Koul wrote:
> 
> +static inline int find_error_code(unsigned int sdw_ret)
> +{
> +	switch (sdw_ret) {
> +	case SDW_CMD_OK:
> +		return 0;
> +
> +	case SDW_CMD_IGNORED:
> +		return -ENODATA;
> +
> +	case SDW_CMD_TIMEOUT:
> +		return -ETIMEDOUT;
> +	}
> +
> +	return -EIO;
> +}
> +
> +static inline int do_transfer(struct sdw_bus *bus,
> +			struct sdw_msg *msg, bool page)
> +{
> +	int retry = bus->prop.err_threshold;
> +	int ret, i;
> +
> +	for (ret = 0, i = 0; i <= retry; i++) {

Initializing ret here is a bit messy.  Better to do it outside.

> +		ret = bus->ops->xfer_msg(bus, msg, page);
> +		ret = find_error_code(ret);
> +		/* if cmd is ok or ignored return */
> +		if (ret == 0 || ret == -ENODATA)
> +			return ret;

Hmm, it's not good to use the same variable for representing two
different things.  Either drop the substitution to ret for
bus->ops->xfer_msg() call, or use another variable to make clear which
one is for SDW_CMD_* and which one is for -EXXX.  The former should be
basically an enum.


> +/**
> + * sdw_transfer: Synchronous transfer message to a SDW Slave device
> + *
> + * @bus: SDW bus
> + * @slave: SDW Slave
> + * @msg: SDW message to be xfered
> + */
> +int sdw_transfer(struct sdw_bus *bus, struct sdw_slave *slave,
> +					struct sdw_msg *msg)
> +{
> +	bool page;
> +	int ret;
> +
> +	mutex_lock(&bus->msg_lock);
> +
> +	page = sdw_get_page(slave, msg);
> +
> +	ret = do_transfer(bus, msg, page);
> +	if (ret != 0 && ret != -ENODATA) {
> +		dev_err(bus->dev, "trf on Slave %d failed:%d\n",
> +				msg->dev_num, ret);
> +		goto error;
> +	}
> +
> +	if (page)
> +		ret = sdw_reset_page(bus, msg->dev_num);
> +
> +error:
> +	mutex_unlock(&bus->msg_lock);
> +
> +	return ret;

So the logic here is that when -ENODATA is returned and page is false,
this function should return -ENODATA to the caller, too, but when page
is set, it returns 0?

> +static inline int sdw_fill_msg(struct sdw_msg *msg, u16 addr,
> +				size_t count, u16 dev_num, u8 flags, u8 *buf)
> +{
> +	msg->addr = (addr >> SDW_REG_SHIFT(SDW_REGADDR));
> +	msg->len = count;
> +	msg->dev_num = dev_num;
> +	msg->addr_page1 = (addr >> SDW_REG_SHIFT(SDW_SCP_ADDRPAGE1_MASK));
> +	msg->addr_page2 = (addr >> SDW_REG_SHIFT(SDW_SCP_ADDRPAGE2_MASK));
> +	msg->flags = flags;
> +	msg->buf = buf;
> +	msg->ssp_sync = false;
> +
> +	return 0;

This function can be void.


thanks,

Takashi