| Message ID | 20230710154932.68377-1-andriy.shevchenko@linux.intel.com |
|---|---|
| Headers | show |
| Series | spi: Header and core clean up and refactoring | expand |
On 10.07.2023 18:49:26, Andy Shevchenko wrote: > Prefer struct_size() over open-coded versions. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > include/linux/spi/spi.h | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > index c9479badf38c..9fb8efb068c6 100644 > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -17,6 +17,7 @@ > #include <linux/minmax.h> > #include <linux/mod_devicetable.h> > #include <linux/mutex.h> > +#include <linux/overflow.h> > #include <linux/scatterlist.h> > #include <linux/slab.h> > #include <linux/smp.h> > @@ -1095,6 +1096,8 @@ struct spi_transfer { > * @state: for use by whichever driver currently owns the message > * @resources: for resource management when the spi message is processed > * @prepared: spi_prepare_message was called for the this message > + * @t: for use with spi_message_alloc() when message and transfers have > + * been allocated together > * > * A @spi_message is used to execute an atomic sequence of data transfers, > * each represented by a struct spi_transfer. The sequence is "atomic" > @@ -1147,6 +1150,9 @@ struct spi_message { > > /* List of spi_res reources when the spi message is processed */ > struct list_head resources; > + > + /* For embedding transfers into the memory of the message */ > + struct spi_transfer t[]; You might want to use the DECLARE_FLEX_ARRAY helper here. Marc
On Mon, Jul 10, 2023 at 07:10:43PM +0300, Andy Shevchenko wrote: > On Mon, Jul 10, 2023 at 05:59:55PM +0200, Marc Kleine-Budde wrote: > > On 10.07.2023 18:49:26, Andy Shevchenko wrote: ... > > > + struct spi_transfer t[]; > > > > You might want to use the DECLARE_FLEX_ARRAY helper here. > > Technically, yes, semantically documentation [1] disagrees with "and [2]" > you, so I leave it as is. > > [1]: Documentation/process/deprecated.rst:269 [2]: Documentation/process/deprecated.rst:350
On Mon, Jul 10, 2023 at 06:49:21PM +0300, Andy Shevchenko wrote: > Since the new spi_controller_xfer_timeout() helper appeared, > we may replace open coded variant in spi_transfer_wait(). > + * Assume speed to be 100 kHz if it's not defined at the time of invocation. > + * You didn't mention this bit in the changelog, and I'm not 100% convinced it was the best idea in the first place. It's going to result in some very big timeouts if it goes off, and we really should be doing validation much earlier in the process. > + u32 speed_hz = xfer->speed_hz ?: 100000; Not only the ternery operator, but the version without the second argument for extra clarity!
On Mon, Jul 10, 2023 at 06:49:17PM +0300, Andy Shevchenko wrote: > Various cleanups and refactorings of the SPI header and core parts > united in a single series. It also touches drivers under SPI subsystem > folder on the pure renaming purposes of some constants. I've queued 1-3, 6-8 and 11- for CI thanks.
On Mon, Jul 10, 2023 at 06:09:00PM +0100, Mark Brown wrote: > On Mon, Jul 10, 2023 at 06:49:19PM +0300, Andy Shevchenko wrote: > > > Refactor spi_register_controller() to drop duplicate IDR allocation. > > Instead of if-else-if branching use two sequential if:s, which allows > > to re-use the logic of IDR allocation in all cases. > > For legibility this should have been split into a separate factoring out > of the shared code and rewriting of the logic, that'd make it trivial to > review. Should I do that for v3? > > - mutex_lock(&board_lock); > > - id = idr_alloc(&spi_master_idr, ctlr, first_dynamic, > > - 0, GFP_KERNEL); > > - mutex_unlock(&board_lock); > > - if (WARN(id < 0, "couldn't get idr")) > > - return id; > > - ctlr->bus_num = id; > > + status = spi_controller_id_alloc(ctlr, first_dynamic, 0); > > + if (status) > > + return status; > > The original does not do the remapping of return codes that the previous > two copies do... Yes, I had to mention this in the commit message that in my opinion this makes no difference. With the dynamically allocated aliases the absence of the slot has the same effect as in the other cases.
On Mon, Jul 10, 2023 at 06:30:32PM +0100, Mark Brown wrote: > On Mon, Jul 10, 2023 at 06:49:21PM +0300, Andy Shevchenko wrote: > > > Since the new spi_controller_xfer_timeout() helper appeared, > > we may replace open coded variant in spi_transfer_wait(). > > > + * Assume speed to be 100 kHz if it's not defined at the time of invocation. > > + * > > You didn't mention this bit in the changelog, and I'm not 100% convinced > it was the best idea in the first place. It's going to result in some > very big timeouts if it goes off, and we really should be doing > validation much earlier in the process. Okay, let's drop this change. > > + u32 speed_hz = xfer->speed_hz ?: 100000; > > Not only the ternery operator, but the version without the second > argument for extra clarity! Elvis can be interpreted as "A _or_ B (if A is false/0)". Some pieces related to SPI use Elvis already IIRC.
On Mon, Jul 10, 2023 at 06:31:12PM +0100, Mark Brown wrote: > On Mon, Jul 10, 2023 at 06:49:17PM +0300, Andy Shevchenko wrote: > > Various cleanups and refactorings of the SPI header and core parts > > united in a single series. It also touches drivers under SPI subsystem > > folder on the pure renaming purposes of some constants. > > I've queued 1-3, 6-8 and 11- for CI thanks. Thank you! Do you think patch 9 deserves to be proceeded?
On Tue, Jul 11, 2023 at 10:28:13AM +0200, AngeloGioacchino Del Regno wrote: > Il 10/07/23 17:49, Andy Shevchenko ha scritto: > > + ms = spi_controller_xfer_timeout(ctlr, xfer); > I agree on using helpers, but the logic is slightly changing here: yes it is > unlikely (and also probably useless) to get ms == UINT_MAX, but the helper is > limiting the maximum timeout value to 500mS, which may not work for some slow > controllers/devices. The helper is limiting the *minimum* timeout value to 500ms - it's using max() not min(). The idea is the other way around, that for a very fast transfer we don't want to end up with such a short timeout that it false triggers due to scheduling issues.
Il 11/07/23 15:05, Mark Brown ha scritto: > On Tue, Jul 11, 2023 at 10:28:13AM +0200, AngeloGioacchino Del Regno wrote: >> Il 10/07/23 17:49, Andy Shevchenko ha scritto: > >>> + ms = spi_controller_xfer_timeout(ctlr, xfer); > >> I agree on using helpers, but the logic is slightly changing here: yes it is >> unlikely (and also probably useless) to get ms == UINT_MAX, but the helper is >> limiting the maximum timeout value to 500mS, which may not work for some slow >> controllers/devices. > > The helper is limiting the *minimum* timeout value to 500ms - it's using > max() not min(). The idea is the other way around, that for a very fast > transfer we don't want to end up with such a short timeout that it false > triggers due to scheduling issues. After reading the code again, yeah, I've totally misread it the first time. Argh! Thanks! :-)
On Tue, Jul 11, 2023 at 02:11:59PM +0300, Andy Shevchenko wrote:
> Do you think patch 9 deserves to be proceeded?
That one I need to think about, may as well resend it and I can think
about the resend.
On Tue, Jul 11, 2023 at 02:38:37PM +0100, Mark Brown wrote: > On Tue, Jul 11, 2023 at 02:11:59PM +0300, Andy Shevchenko wrote: > > > Do you think patch 9 deserves to be proceeded? > > That one I need to think about, may as well resend it and I can think > about the resend. Got it. Probably I have to amend commit message in the patch 9 to point out why struct_size() is better.
On Tue, Jul 11, 2023 at 02:01:13PM +0300, Andy Shevchenko wrote: > On Mon, Jul 10, 2023 at 06:30:32PM +0100, Mark Brown wrote: > > On Mon, Jul 10, 2023 at 06:49:21PM +0300, Andy Shevchenko wrote: > > > + * Assume speed to be 100 kHz if it's not defined at the time of invocation. > > You didn't mention this bit in the changelog, and I'm not 100% convinced > > it was the best idea in the first place. It's going to result in some > > very big timeouts if it goes off, and we really should be doing > > validation much earlier in the process. > Okay, let's drop this change. Like I say we *should* be fine with the refactoring without this, or at least if it's an issue we should improve the validation. > > > + u32 speed_hz = xfer->speed_hz ?: 100000; > > Not only the ternery operator, but the version without the second > > argument for extra clarity! > Elvis can be interpreted as "A _or_ B (if A is false/0)". > Some pieces related to SPI use Elvis already IIRC. I understand what it means, I just don't find it's adding clarity most of the times it's used (there's a few places where it is useful like pasting in strings in formats). There are some examples that I'd complain about in the code, most of them predating me working on SPI too much, but I'm not a fan.
On Tue, Jul 11, 2023 at 03:14:54PM +0100, Mark Brown wrote: > On Tue, Jul 11, 2023 at 02:01:13PM +0300, Andy Shevchenko wrote: > > On Mon, Jul 10, 2023 at 06:30:32PM +0100, Mark Brown wrote: > > > On Mon, Jul 10, 2023 at 06:49:21PM +0300, Andy Shevchenko wrote: > > > > > + * Assume speed to be 100 kHz if it's not defined at the time of invocation. > > > > You didn't mention this bit in the changelog, and I'm not 100% convinced > > > it was the best idea in the first place. It's going to result in some > > > very big timeouts if it goes off, and we really should be doing > > > validation much earlier in the process. > > > Okay, let's drop this change. > > Like I say we *should* be fine with the refactoring without this, or at > least if it's an issue we should improve the validation. For the speeds < 1000 Hz, this change will lead to the div by 0 crash. It seems that the current code which this one removes is better than the spi_controller_xfer_timeout() provides. If anything, the spi_controller_xfer_timeout() should be improved first. So, for now I drop this for sure. Maybe in the future we can come back to it.
On Tue, Jul 11, 2023 at 06:30:06PM +0300, Andy Shevchenko wrote: > On Tue, Jul 11, 2023 at 03:14:54PM +0100, Mark Brown wrote: > > Like I say we *should* be fine with the refactoring without this, or at > > least if it's an issue we should improve the validation. > For the speeds < 1000 Hz, this change will lead to the div by 0 crash. > It seems that the current code which this one removes is better than > the spi_controller_xfer_timeout() provides. > If anything, the spi_controller_xfer_timeout() should be improved first. > So, for now I drop this for sure. Maybe in the future we can come back > to it. I don't think this is the only thing that might fall over without a speed, what we've generally been doing (and do try to do with speeds, we already need to default in the controller's speed and so on) is to sanitise input on the way into the subsystem rather than trying to ensure that all the users are handling everything.
On Mon, 10 Jul 2023 18:49:17 +0300, Andy Shevchenko wrote: > Various cleanups and refactorings of the SPI header and core parts > united in a single series. It also touches drivers under SPI subsystem > folder on the pure renaming purposes of some constants. > > No functional change intended. > > Changelog v2: > - added new patches 3,4,5,10,13,14 > - massaged comment and kernel doc in patch 9 > - split used to be patch 4 to patches 11,12 > - covered a few things in SPI core in patch 15 > - amended commit message for above (Mark) > - reshuffled patches in the series for better logical grouping > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [01/15] spi: Remove unneeded OF node NULL checks commit: fbab5b2c09060e8034fee6ec2df69a62594fb7db [02/15] spi: Drop duplicate IDR allocation code in spi_register_controller() commit: 440c47331bdb889e24128c75387c695ca81d9b9b [03/15] spi: Replace if-else-if by bitops and multiplications commit: 2b308e7176e366a52a07a49868e3b1a295e56785 [06/15] spi: Use sysfs_emit() to instead of s*printf() commit: f2daa4667fda1aa951b91da0ae9675a5da9d7716 [07/15] spi: Sort headers alphabetically commit: edf6a864c996f9a9f5299a3b3e574a37e64000c5 [08/15] spi: Clean up headers (no commit info) [11/15] spi: Get rid of old SPI_MASTER_NO_TX & SPI_MASTER_NO_RX commit: c397f09e5498994790503a64486213ef85e58db9 [12/15] spi: Get rid of old SPI_MASTER_MUST_TX & SPI_MASTER_MUST_RX commit: 90366cd60133a9f5b6a2f31360367c658585e125 [13/15] spi: Rename SPI_MASTER_GPIO_SS to SPI_CONTROLLER_GPIO_SS commit: 82238d2cbd99ebd09dda48fb7c1c8802097da6a2 [14/15] spi: Convert to SPI_CONTROLLER_HALF_DUPLEX commit: 7a2b552c8e0e5bb280558f6c120140f5f06323bc [15/15] spi: Fix spelling typos and acronyms capitalization commit: 702ca0269ed56e2d8dae7874a4d8af268e2a382e All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark