linux-kernel - Re: [PATCH] spi: SuperH MSIOF SPI Master driver

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 26 Nov 2009 15:43:16 +0900
From:	Paul Mundt <lethal@...ux-sh.org>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Magnus Damm <magnus.damm@...il.com>,
	spi-devel-general@...ts.sourceforge.net,
	dbrownell@...rs.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] spi: SuperH MSIOF SPI Master driver

On Wed, Nov 25, 2009 at 12:11:52PM -0800, Andrew Morton wrote:
> On Tue, 24 Nov 2009 21:55:31 +0900
> Magnus Damm <magnus.damm@...il.com> wrote:
> > +struct sh_msiof_spi_priv {
> > +	struct spi_bitbang bitbang; /* must be first for spi_bitbang.c */
> 
> Well if that's the case then spi_bitbang.c needs smacking.  What causes
> this requirement?
> 
spi_bitbang causes this requirement by being lazy with regards to
private data stashing. Both the drivers and the bitbang code use
spi_master_get_devdata() for getting their private data, which wraps
in to a dev_get_drvdata().

This needs to be reworked so that struct spi_master has its own private
data pointer which helper code like spi_bitbang can use, while freeing up
the struct device private data pointer so it can be freely used by
drivers as normal.

This is the same sort of private data through casting whimsy that the
framebuffer drivers used to employ, only fortunately that behaviour never
made it out of 2.3.x kernels -- until now!

> > +	void __iomem *mapbase;
> > +	struct clk *clk;
> > +	struct platform_device *pdev;
> > +	struct sh_msiof_spi_info *info;
> > +	struct completion done;
> > +	unsigned long flags;
> > +	int tx_fifo_size;
> > +	int rx_fifo_size;
> > +};
> > +
> >
> > ...
> >
> > +static void sh_msiof_modify_ctr_wait(struct sh_msiof_spi_priv *p,
> > +				     unsigned long clr, unsigned long set)
> > +{
> > +	unsigned long mask = clr | set;
> > +	unsigned long data;
> > +
> > +	data = sh_msiof_read(p, CTR);
> > +	data &= ~clr;
> > +	data |= set;
> > +	sh_msiof_write(p, CTR, data);
> > +
> > +	while ((sh_msiof_read(p, CTR) & mask) != set)
> > +		;
> 
> hm, confidence.  No timeout needed here?
> 
This definitely needs a timeout, nothing involving SPI inspires
confidence. A cpu_relax() to prevent the compiler from optimizing the
loop out would help, too.
--
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/