Re: [PATCH AUTOSEL for 4.9 002/219] spi/bcm63xx: make spi subsystem aware of message size limits

[Mark Brown]

On Tue, Mar 06, 2018 at 04:33:34PM +0100, Jonas Gorski wrote:

> I would assume that this is true for most systems with a limited SPI
> controller. I would hope that most board designers are sensible enough
> to not add devices that won't work ;-)

With things like firmware downloads the problem is that they can
generally work, they're just horribly inefficient since you have to
chunk them up into multiple operations rather than just pointing the
hardware at a big buffer and letting it get on with things (same problem
as with flash really).


signature.asc
Description: PGP signature

Re: [PATCH AUTOSEL for 4.9 002/219] spi/bcm63xx: make spi subsystem aware of message size limits

[Mark Brown]

On Tue, Mar 06, 2018 at 04:33:34PM +0100, Jonas Gorski wrote:

> I would assume that this is true for most systems with a limited SPI
> controller. I would hope that most board designers are sensible enough
> to not add devices that won't work ;-)

With things like firmware downloads the problem is that they can
generally work, they're just horribly inefficient since you have to
chunk them up into multiple operations rather than just pointing the
hardware at a big buffer and letting it get on with things (same problem
as with flash really).


signature.asc
Description: PGP signature

Re: [PATCH AUTOSEL for 4.9 002/219] spi/bcm63xx: make spi subsystem aware of message size limits

[Jonas Gorski]

On 6 March 2018 at 15:20, Mark Brown  wrote:
> On Tue, Mar 06, 2018 at 02:42:43PM +0100, Jonas Gorski wrote:
>> On 5 March 2018 at 21:35, Mark Brown  wrote:
>
>> > It's exposing more capability information but it's in the "how did this
>> > ever work without the fix" range, and I'd worry that this might cause us
>> > to do something like start exercising handling code in client drivers
>> > that had never been tested.  Not that I can immediately see any client
>> > drivers in mainline that actually pay attention...  :/
>
>> I would assume that most spi client drivers use short messages, so
>> they aren't affected unless the max message size is really short.
>> m25p80 on the other hand will do arbitrarily large transfers/reads, so
>> there it was supported first.
>
> There's a bunch of SPI drivers that do firmware downloads which I'd
> expect to be affected, the limit the device has is tiny so it's
> relatively easy to bump into it.  It's very rare for devices to be so
> limited so unfortunately client drivers don't generally check though.

Well, at least for bcm63xx it's very rare to have something other than
a flash chip, a (broadcom) ethernet switch management interface, or a
SLIC/SLAC attached to the SPI controller. And AFAICT of these three
only the flash chip uses large SPI transfers. Furthermore, unless you
have a development board, you won't be able to attach anything
different to it. So the chance to bump into the limits with other
drivers is rather low.

I would assume that this is true for most systems with a limited SPI
controller. I would hope that most board designers are sensible enough
to not add devices that won't work ;-)


Regards
Jonas

Re: [PATCH AUTOSEL for 4.9 002/219] spi/bcm63xx: make spi subsystem aware of message size limits

[Jonas Gorski]

On 6 March 2018 at 15:20, Mark Brown  wrote:
> On Tue, Mar 06, 2018 at 02:42:43PM +0100, Jonas Gorski wrote:
>> On 5 March 2018 at 21:35, Mark Brown  wrote:
>
>> > It's exposing more capability information but it's in the "how did this
>> > ever work without the fix" range, and I'd worry that this might cause us
>> > to do something like start exercising handling code in client drivers
>> > that had never been tested.  Not that I can immediately see any client
>> > drivers in mainline that actually pay attention...  :/
>
>> I would assume that most spi client drivers use short messages, so
>> they aren't affected unless the max message size is really short.
>> m25p80 on the other hand will do arbitrarily large transfers/reads, so
>> there it was supported first.
>
> There's a bunch of SPI drivers that do firmware downloads which I'd
> expect to be affected, the limit the device has is tiny so it's
> relatively easy to bump into it.  It's very rare for devices to be so
> limited so unfortunately client drivers don't generally check though.

Well, at least for bcm63xx it's very rare to have something other than
a flash chip, a (broadcom) ethernet switch management interface, or a
SLIC/SLAC attached to the SPI controller. And AFAICT of these three
only the flash chip uses large SPI transfers. Furthermore, unless you
have a development board, you won't be able to attach anything
different to it. So the chance to bump into the limits with other
drivers is rather low.

I would assume that this is true for most systems with a limited SPI
controller. I would hope that most board designers are sensible enough
to not add devices that won't work ;-)


Regards
Jonas

Re: [PATCH AUTOSEL for 4.9 002/219] spi/bcm63xx: make spi subsystem aware of message size limits

[Mark Brown]

On Tue, Mar 06, 2018 at 02:42:43PM +0100, Jonas Gorski wrote:
> On 5 March 2018 at 21:35, Mark Brown  wrote:

> > It's exposing more capability information but it's in the "how did this
> > ever work without the fix" range, and I'd worry that this might cause us
> > to do something like start exercising handling code in client drivers
> > that had never been tested.  Not that I can immediately see any client
> > drivers in mainline that actually pay attention...  :/

> I would assume that most spi client drivers use short messages, so
> they aren't affected unless the max message size is really short.
> m25p80 on the other hand will do arbitrarily large transfers/reads, so
> there it was supported first.

There's a bunch of SPI drivers that do firmware downloads which I'd
expect to be affected, the limit the device has is tiny so it's
relatively easy to bump into it.  It's very rare for devices to be so
limited so unfortunately client drivers don't generally check though.


signature.asc
Description: PGP signature

Re: [PATCH AUTOSEL for 4.9 002/219] spi/bcm63xx: make spi subsystem aware of message size limits

[Mark Brown]

On Tue, Mar 06, 2018 at 02:42:43PM +0100, Jonas Gorski wrote:
> On 5 March 2018 at 21:35, Mark Brown  wrote:

> > It's exposing more capability information but it's in the "how did this
> > ever work without the fix" range, and I'd worry that this might cause us
> > to do something like start exercising handling code in client drivers
> > that had never been tested.  Not that I can immediately see any client
> > drivers in mainline that actually pay attention...  :/

> I would assume that most spi client drivers use short messages, so
> they aren't affected unless the max message size is really short.
> m25p80 on the other hand will do arbitrarily large transfers/reads, so
> there it was supported first.

There's a bunch of SPI drivers that do firmware downloads which I'd
expect to be affected, the limit the device has is tiny so it's
relatively easy to bump into it.  It's very rare for devices to be so
limited so unfortunately client drivers don't generally check though.


signature.asc
Description: PGP signature

Re: [PATCH AUTOSEL for 4.9 002/219] spi/bcm63xx: make spi subsystem aware of message size limits

[Jonas Gorski]

On 5 March 2018 at 21:35, Mark Brown  wrote:
> On Mon, Mar 05, 2018 at 08:07:46PM +, Sasha Levin wrote:
>> On Mon, Mar 05, 2018 at 10:23:10AM +, Mark Brown wrote:
>> >On Sat, Mar 03, 2018 at 10:27:56PM +, Sasha Levin wrote:
>
>> >> The bcm63xx SPI controller does not allow manual control of the CS
>> >> lines and will toggle it automatically before and after sending data,
>> >> so we are limited to messages that fit in the FIFO buffer. Since the CS
>> >> lines aren't available as GPIOs either, we will need to make slave
>> >> drivers aware of this limitation so they can handle them accordingly.
>
>> >This seems really aggressive for stable...
>
>> Why so?
>
> It's exposing more capability information but it's in the "how did this
> ever work without the fix" range, and I'd worry that this might cause us
> to do something like start exercising handling code in client drivers
> that had never been tested.  Not that I can immediately see any client
> drivers in mainline that actually pay attention...  :/

I would assume that most spi client drivers use short messages, so
they aren't affected unless the max message size is really short.
m25p80 on the other hand will do arbitrarily large transfers/reads, so
there it was supported first.

m25p80 supports max_transfer_size since 4,9, and max_message_size
since 4.11 with commit 9e276de6a367cde07c1a63522152985d4e5cca8b. So
that one would need to be backported as well for the max_message_size
being actually meaningful.

tinydrm-helpers also observes max_transfers_size since 4.11 with
commit 9f69eb5c36a644571cca6b2f8dc5f6a7cba04a8b where it was added,
but since this is a larger commit and not just a "bugfix" one, this
doesn't seem like a candidate for backporting.


Regards
Jonas

Re: [PATCH AUTOSEL for 4.9 002/219] spi/bcm63xx: make spi subsystem aware of message size limits

[Jonas Gorski]

On 5 March 2018 at 21:35, Mark Brown  wrote:
> On Mon, Mar 05, 2018 at 08:07:46PM +, Sasha Levin wrote:
>> On Mon, Mar 05, 2018 at 10:23:10AM +, Mark Brown wrote:
>> >On Sat, Mar 03, 2018 at 10:27:56PM +, Sasha Levin wrote:
>
>> >> The bcm63xx SPI controller does not allow manual control of the CS
>> >> lines and will toggle it automatically before and after sending data,
>> >> so we are limited to messages that fit in the FIFO buffer. Since the CS
>> >> lines aren't available as GPIOs either, we will need to make slave
>> >> drivers aware of this limitation so they can handle them accordingly.
>
>> >This seems really aggressive for stable...
>
>> Why so?
>
> It's exposing more capability information but it's in the "how did this
> ever work without the fix" range, and I'd worry that this might cause us
> to do something like start exercising handling code in client drivers
> that had never been tested.  Not that I can immediately see any client
> drivers in mainline that actually pay attention...  :/

I would assume that most spi client drivers use short messages, so
they aren't affected unless the max message size is really short.
m25p80 on the other hand will do arbitrarily large transfers/reads, so
there it was supported first.

m25p80 supports max_transfer_size since 4,9, and max_message_size
since 4.11 with commit 9e276de6a367cde07c1a63522152985d4e5cca8b. So
that one would need to be backported as well for the max_message_size
being actually meaningful.

tinydrm-helpers also observes max_transfers_size since 4.11 with
commit 9f69eb5c36a644571cca6b2f8dc5f6a7cba04a8b where it was added,
but since this is a larger commit and not just a "bugfix" one, this
doesn't seem like a candidate for backporting.


Regards
Jonas

Re: [PATCH AUTOSEL for 4.9 002/219] spi/bcm63xx: make spi subsystem aware of message size limits

[Mark Brown]

On Mon, Mar 05, 2018 at 08:07:46PM +, Sasha Levin wrote:
> On Mon, Mar 05, 2018 at 10:23:10AM +, Mark Brown wrote:
> >On Sat, Mar 03, 2018 at 10:27:56PM +, Sasha Levin wrote:

> >> The bcm63xx SPI controller does not allow manual control of the CS
> >> lines and will toggle it automatically before and after sending data,
> >> so we are limited to messages that fit in the FIFO buffer. Since the CS
> >> lines aren't available as GPIOs either, we will need to make slave
> >> drivers aware of this limitation so they can handle them accordingly.

> >This seems really aggressive for stable...

> Why so?

It's exposing more capability information but it's in the "how did this
ever work without the fix" range, and I'd worry that this might cause us
to do something like start exercising handling code in client drivers
that had never been tested.  Not that I can immediately see any client
drivers in mainline that actually pay attention...  :/


signature.asc
Description: PGP signature

Re: [PATCH AUTOSEL for 4.9 002/219] spi/bcm63xx: make spi subsystem aware of message size limits

[Mark Brown]

On Mon, Mar 05, 2018 at 08:07:46PM +, Sasha Levin wrote:
> On Mon, Mar 05, 2018 at 10:23:10AM +, Mark Brown wrote:
> >On Sat, Mar 03, 2018 at 10:27:56PM +, Sasha Levin wrote:

> >> The bcm63xx SPI controller does not allow manual control of the CS
> >> lines and will toggle it automatically before and after sending data,
> >> so we are limited to messages that fit in the FIFO buffer. Since the CS
> >> lines aren't available as GPIOs either, we will need to make slave
> >> drivers aware of this limitation so they can handle them accordingly.

> >This seems really aggressive for stable...

> Why so?

It's exposing more capability information but it's in the "how did this
ever work without the fix" range, and I'd worry that this might cause us
to do something like start exercising handling code in client drivers
that had never been tested.  Not that I can immediately see any client
drivers in mainline that actually pay attention...  :/


signature.asc
Description: PGP signature

Re: [PATCH AUTOSEL for 4.9 002/219] spi/bcm63xx: make spi subsystem aware of message size limits

[Sasha Levin]

On Mon, Mar 05, 2018 at 10:23:10AM +, Mark Brown wrote:
>On Sat, Mar 03, 2018 at 10:27:56PM +, Sasha Levin wrote:
>> From: Jonas Gorski 
>>
>> [ Upstream commit 0135c03df914f0481c61f097c78d37cece84f330 ]
>
>Why are there so many more patches for v4.9 than for more recent
>kernels?

The v4.11..v4.14 range got processed, and all those commits are now
being pushed into 4.9 and older.

>> The bcm63xx SPI controller does not allow manual control of the CS
>> lines and will toggle it automatically before and after sending data,
>> so we are limited to messages that fit in the FIFO buffer. Since the CS
>> lines aren't available as GPIOs either, we will need to make slave
>> drivers aware of this limitation so they can handle them accordingly.
>
>This seems really aggressive for stable...

Why so?

-- 

Thanks,
Sasha

Re: [PATCH AUTOSEL for 4.9 002/219] spi/bcm63xx: make spi subsystem aware of message size limits

[Sasha Levin]

On Mon, Mar 05, 2018 at 10:23:10AM +, Mark Brown wrote:
>On Sat, Mar 03, 2018 at 10:27:56PM +, Sasha Levin wrote:
>> From: Jonas Gorski 
>>
>> [ Upstream commit 0135c03df914f0481c61f097c78d37cece84f330 ]
>
>Why are there so many more patches for v4.9 than for more recent
>kernels?

The v4.11..v4.14 range got processed, and all those commits are now
being pushed into 4.9 and older.

>> The bcm63xx SPI controller does not allow manual control of the CS
>> lines and will toggle it automatically before and after sending data,
>> so we are limited to messages that fit in the FIFO buffer. Since the CS
>> lines aren't available as GPIOs either, we will need to make slave
>> drivers aware of this limitation so they can handle them accordingly.
>
>This seems really aggressive for stable...

Why so?

-- 

Thanks,
Sasha

Re: [PATCH AUTOSEL for 4.9 002/219] spi/bcm63xx: make spi subsystem aware of message size limits

[Mark Brown]

On Sat, Mar 03, 2018 at 10:27:56PM +, Sasha Levin wrote:
> From: Jonas Gorski 
> 
> [ Upstream commit 0135c03df914f0481c61f097c78d37cece84f330 ]

Why are there so many more patches for v4.9 than for more recent
kernels?

> The bcm63xx SPI controller does not allow manual control of the CS
> lines and will toggle it automatically before and after sending data,
> so we are limited to messages that fit in the FIFO buffer. Since the CS
> lines aren't available as GPIOs either, we will need to make slave
> drivers aware of this limitation so they can handle them accordingly.

This seems really aggressive for stable...


signature.asc
Description: PGP signature

Re: [PATCH AUTOSEL for 4.9 002/219] spi/bcm63xx: make spi subsystem aware of message size limits

[Mark Brown]

On Sat, Mar 03, 2018 at 10:27:56PM +, Sasha Levin wrote:
> From: Jonas Gorski 
> 
> [ Upstream commit 0135c03df914f0481c61f097c78d37cece84f330 ]

Why are there so many more patches for v4.9 than for more recent
kernels?

> The bcm63xx SPI controller does not allow manual control of the CS
> lines and will toggle it automatically before and after sending data,
> so we are limited to messages that fit in the FIFO buffer. Since the CS
> lines aren't available as GPIOs either, we will need to make slave
> drivers aware of this limitation so they can handle them accordingly.

This seems really aggressive for stable...


signature.asc
Description: PGP signature