linux-kernel - Re: [spi-devel-general] [PATCH 1/2] spi: new SPI bus lock/unlockfunctions

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:	Sat, 19 Sep 2009 15:11:59 -0400
From:	Mike Frysinger <vapier.adi@...il.com>
To:	H Hartley Sweeten <hartleys@...ionengravers.com>
Cc:	spi-devel-general@...ts.sourceforge.net,
	David Brownell <dbrownell@...rs.sourceforge.net>,
	Yi Li <yi.li@...log.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [spi-devel-general] [PATCH 1/2] spi: new SPI bus 
	lock/unlockfunctions

On Fri, Sep 18, 2009 at 19:00, H Hartley Sweeten wrote:
> On Thursday, September 17, 2009 3:54 PM, Mike Frysinger wrote:
>>> I assume the spi master driver must supply the {lock/unlock}_bus methods
>>> to properly support the locking.
>>
>> currently, yes.  a common solution would be nice.  ideas/patches welcome ;).
>>
>>> But, by returning 0 when the methods
>>> are not supplied you are basically saying all the current master drivers
>>> in mainline support bus locking.  I think this is really only "true" if
>>> spi->master->num_chipselect == 1.
>>
>> right, but that is no different from what we have today.  there is no
>> way for a client to guarantee exclusive access, so you cant write code
>> assuming it in the first place.  the only consumer thus far (mmc_spi)
>> actually errors out if such conditions exist.
>>
>> in other words, we arent breaking anything.
>
> Actually you are breaking the mmc_spi driver.

no we arent.  the current code does some sloppy checking for possible
concurrent access and then aborts.  the new code drops that sloppy
checking in place of the bus master doing it right.  any setup that is
working today will continue to work fine.  and setup *that is already
broken* will continue to be broken.

> By returning 0 when the methods are not supplied you are saying that the
> master driver supports and locked the bus.  At a minimum, I think spi_lock_bus()
> should return an error code if locking is not supported.
>
> Also, as Andrew Morton pointed out, calling spi_unlock_bus() without having
> a valid lock by calling spi_lock_bus() is a bug.
>
> In addition your patch to mmc_spi should check the return code from
> spi_lock_bus().  If the driver "requires" that the bus be locked it should
> trigger an error path if it cannot be locked.
>
>>> Also, do you have a master driver that does have the {lock/unlock}_bus
>>> methods?  I would like to see how you handled it.
>>
>> http://git.kernel.org/?p=linux/kernel/git/vapier/blackfin.git;a=commitdiff;h=cc54fa8ed63e11a000031bc650d41d57b441803d
>
> Oiy... The lock/unlock functions are simple enough but the change needed
> to bfin_spi_pump_messages() is a bit complicated.
>
> What happens to next_msg if it is for other devices on the bus?

Yi can take a stab at addressing these issues since he is the guy who
has been putting together in the first place.
-mike
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/