linux-kernel - Re: [RFC 7/7] [media] rc: add support for IR LEDs driven through SPI

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, 21 Jul 2016 10:09:26 +0900
From:	Andi Shyti <andi.shyti@...sung.com>
To:	Sean Young <sean@...s.org>
Cc:	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	Andi Shyti <andi@...zian.org>
Subject: Re: [RFC 7/7] [media] rc: add support for IR LEDs driven through SPI

Hi Sean,

> > +	int ret;
> > +	struct ir_spi_data *idata = (struct ir_spi_data *) dev->priv;
> 
> No cast needed.

yes, thanks.

> > +	ret = regulator_enable(idata->regulator);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mutex_lock(&idata->mutex);
> > +	idata->xfer.len = n;
> > +	idata->xfer.tx_buf = buffer;
> > +	mutex_unlock(&idata->mutex);
> 
> I'm not convinced the locking works here. You want to guard against 
> someone modifying xfer while you are sending (so in spi_sync_transfer), 
> which this locking is not doing. You could declare a 
> local "struct spi_transfer xfer" and avoid the mutex altogether.

I cannot declare xfer locally because the spi framework needs
a statically allocated xfer, so that either I dynamically
allocate it in the function or I declare it global in idata.

With the mutex I would like to prevent different tasks to change
the value at the same time, it's an easy case, it shouldn't make
much difference.

There are checkpatch issues, in the next patchset I will fix
them.

Thanks a lot for your review,
Andi