linux-kernel - Re: [PATCH] ad7877: fix spi word size to 16 bit

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: <AANLkTimeAn9ALs4KxBNwwk-B419OwZ0BUwoKcoEfUbGw@mail.gmail.com>
Date:	Mon, 17 May 2010 19:49:46 -0400
From:	Mike Frysinger <vapier.adi@...il.com>
To:	Oskar Schirmer <os@...ix.com>
Cc:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Daniel Glöckner <dg@...ix.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
	Oliver Schneidewind <osw@...ix.com>,
	Michael Hennerich <michael.hennerich@...log.com>
Subject: Re: [PATCH] ad7877: fix spi word size to 16 bit

2010/5/17 Oskar Schirmer:
> On Sun, May 16, 2010 at 15:25:34 -0400, Mike Frysinger wrote:
>> On Sat, May 15, 2010 at 14:15, Oskar Schirmer wrote:
>> > On Thu, May 13, 2010 at 00:53:35 -0700, Dmitry Torokhov wrote:
>> >> On Fri, May 07, 2010 at 02:23:07PM -0400, Mike Frysinger wrote:
>> >> > On Fri, May 7, 2010 at 05:41, Daniel Glöckner wrote:
>> >> > > On 05/06/2010 08:26 PM, Mike Frysinger wrote:
>> >> > >> i think it'd be a better idea to do something like:
>> >> > >>   if (spi->bits_per_word != 16) {
>> >> > >>     if (spi->bits_per_word) {
>> >> > >>       dev_err(&spi->dev, "Invalid SPI settings; bits_per_word must be 16\n");
>> >> > >>       return -EINVAL;
>> >> > >>     }
>> >> > >>     spi->bits_per_word = 16;
>> >> > >>     spi_setup(spi);
>> >> > >>   }
>> >> > >
>> >> > > There is no way to set bits_per_word using struct spi_board_info. The
>> >> > > description of that structure in spi.h explicitly lists the wordsize as
>> >> > > one of the parameters drivers should set themself in probe().
>> >> > >
>> >> > > Only struct bfin5xx_spi_chip allows to set this value in the board code.
>> >> >
>> >> > an obvious shortcoming in the SPI framework that should be fixed, but
>> >> > that doesnt make any difference to the above code now does it ?  it'll
>> >> > operate correctly regardless of the SPI bus master.
>> >>
>> >> So is the updated patch coming?
>> >
>> > The basic question I see is, whether it is in the
>> > responsibility of ad7877 to check a wrong setting
>> > possibly caused in board specific code. If so,
>> > then the proposal by Mike should be used, but if not
>> > so, it would introduce unneeded code.
>> >
>> > Remember: both versions end up in correctly setting
>> > bits_per_word, with the difference merely in feedback
>> > level.
>>
>> imo, unsupported board settings should always be detected & rejected.
>> all SPI master drivers do this (detect & reject unsupported SPI slave
>> settings).
>
> please note, that bits_per_word is not a board setting,
> it's a demand of the device. consequently, there is no one
> to set unsupported values and thus none to be detected.
>
> the only architecture setting bits_per_word thru spi_chip
> is blackfin, but I cannot see a good reason, why the board
> settings should engage with a fixed demand of the device?

you're telling me that every single SPI device out there can only ever
operate in one specific bit length or lacks optional settings ?  i
find that hard to believe which means it does make sense to let the
boards pick a length when appropriate.

the board settings also function as default setup values so when using
generic things like the spidev driver, there is no kernel driver to
request the desired bitmode.

the simple code i posted addresses your concerns as well as reject
invalid settings (wherever they may originate).  if you're not going
to post an updated patch, i'll take the simple one Michael committed
and post that.
-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/