| Message ID | 20201209173514.93328-1-tudor.ambarus@microchip.com |
|---|---|
| State | Accepted |
| Commit | 9326e4f1e5dd1a4410c429638d3c412b6fc17040 |
| Headers | show |
| Series | spi: Limit the spi device max speed to controller's max speed | expand |
On Wed, Dec 09, 2020 at 10:46:36PM +0300, Serge Semin wrote: > On Wed, Dec 09, 2020 at 07:35:14PM +0200, Tudor Ambarus wrote: > > Make sure the max_speed_hz of spi_device does not override > > the max_speed_hz of controller. > I have doubts that's right thing to do. It seems better to let > the controller driver to handle the speed clamping itself, while > to leave the SPI client device max_speed_hz field describing the > device speed capability. Moreover the SPI-transfers passed to the > controller will have a SPI-bus speed fixed in accordance with the > controller and client device capabilities anyway. > See the __spi_validate() method for details: > https://elixir.bootlin.com/linux/v5.10-rc7/source/drivers/spi/spi.c#L3570 Right, in general we aim to do this sort of fixup on the transfers and messages rather than the devices, I guess we might be missing validation in some of the flash acceleration paths or was this an issue seen through inspection?
On Wed, Dec 09, 2020 at 07:54:20PM +0000, Mark Brown wrote: > On Wed, Dec 09, 2020 at 10:46:36PM +0300, Serge Semin wrote: > > > On Wed, Dec 09, 2020 at 07:35:14PM +0200, Tudor Ambarus wrote: > > > > Make sure the max_speed_hz of spi_device does not override > > > the max_speed_hz of controller. > > > I have doubts that's right thing to do. It seems better to let > > the controller driver to handle the speed clamping itself, while > > to leave the SPI client device max_speed_hz field describing the > > device speed capability. Moreover the SPI-transfers passed to the > > controller will have a SPI-bus speed fixed in accordance with the > > controller and client device capabilities anyway. > > See the __spi_validate() method for details: > > https://elixir.bootlin.com/linux/v5.10-rc7/source/drivers/spi/spi.c#L3570 > > Right, in general we aim to do this sort of fixup on the transfers > and messages rather than the devices, I guess we might be missing > validation in some of the flash acceleration paths or was this an issue > seen through inspection? In case of DW SPI driver we just make sure the SPI-client device speed set in the max_speed_hz doesn't exceed the controller SPI-bus clock frequency and clamp it if it does. So the driver is safe in that matter. -Sergey
Hi, Serge, Mark, On 12/9/20 10:30 PM, Serge Semin wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Wed, Dec 09, 2020 at 08:25:52PM +0000, Mark Brown wrote: >> On Wed, Dec 09, 2020 at 11:15:35PM +0300, Serge Semin wrote: >>> On Wed, Dec 09, 2020 at 07:54:20PM +0000, Mark Brown wrote: >> >>>> Right, in general we aim to do this sort of fixup on the transfers >>>> and messages rather than the devices, I guess we might be missing >>>> validation in some of the flash acceleration paths or was this an issue >>>> seen through inspection? We miss validation for the SPI controllers that provide the spi_controller_mem_ops with its exec_op() method. In this case the SPI core does not check if the max_speed_hz of spi_device overrides the max_speed_hz of controller. This was seen through inspection. There are octal SPI NOR flashes that can run at 400 MHZ, and I've also seen SPI controllers that are limited to 200 MHZ (microchip's sama7g5 octal SPI for example, which is not yet in mainline). >> >>> In case of DW SPI driver we just make sure the SPI-client device >>> speed set in the max_speed_hz doesn't exceed the controller SPI-bus >>> clock frequency and clamp it if it does. So the driver is safe in that >>> matter. >> > >> Ideally the driver wouldn't have to check though (no harm in doing so of >> course). Right. I thought of doing this in the SPI core, rather than doing in (each) controller driver. Serge, I've looked at https://elixir.bootlin.com/linux/v5.10-rc7/source/drivers/spi/spi.c#L3570 That limits the xfer speed hz to the controller's max_speed_hz. SPI controllers that implement the exec_op() optimized handler are not going through this path and are not covered by the check in __spi_validate(). > > If so then we'd need to have a dedicated speed-related field in the > spi_mem_op structure, which would be accordingly initialized by the > SPI-mem core. > We do need a max_speed_hz in the SPIMEM, but probably for other purposes: NOR flashes, for example, can discover the maximum supported frequency at run-time, when parsing the jesd216 SFDP tables. One may consider that the run-time discovered max_speed_hz should have a higher precedence than what we fill with the spi-max-frequency dt property, because the manufacturers/jesd216 standard know/s better. Of course, if the manufacturers put a wrong max_speed_hz in the sfdp table, that can be updated at runtime with a fixup() hook, on a case by case basis. Other thing to consider here, is the max_speed_hz supported by the PCB. For example if the SPI flash supports 400 MHZ, the controller 200 MHZ, but the PCB only 180 MHZ, then we'll have to synchronize all three. But this seems like a discussion for other patch. Let me know if you think that this patch is ok for now. I can update the commit message if needed. Cheers, ta
On Thu, Dec 10, 2020 at 08:58:18AM +0000, Tudor.Ambarus@microchip.com wrote: > On 12/9/20 10:30 PM, Serge Semin wrote: > >>>> Right, in general we aim to do this sort of fixup on the transfers > >>>> and messages rather than the devices, I guess we might be missing > >>>> validation in some of the flash acceleration paths or was this an issue > >>>> seen through inspection? > We miss validation for the SPI controllers that provide the > spi_controller_mem_ops with its exec_op() method. In this case the SPI > core does not check if the max_speed_hz of spi_device overrides the > max_speed_hz of controller. > This was seen through inspection. There are octal SPI NOR flashes that > can run at 400 MHZ, and I've also seen SPI controllers that are limited > to 200 MHZ (microchip's sama7g5 octal SPI for example, which is not yet > in mainline). Right, that's the hole :/ > >> Ideally the driver wouldn't have to check though (no harm in doing so of > >> course). > Right. I thought of doing this in the SPI core, rather than doing in (each) > controller driver. Yes, we should just make sure things are OK in the core as much as we can so there's less work for driver authors. > > If so then we'd need to have a dedicated speed-related field in the > > spi_mem_op structure, which would be accordingly initialized by the > > SPI-mem core. > We do need a max_speed_hz in the SPIMEM, but probably for other purposes: > NOR flashes, for example, can discover the maximum supported frequency > at run-time, when parsing the jesd216 SFDP tables. One may consider that > the run-time discovered max_speed_hz should have a higher precedence than > what we fill with the spi-max-frequency dt property, because the > manufacturers/jesd216 standard know/s better. Of course, if the > manufacturers put a wrong max_speed_hz in the sfdp table, that can be > updated at runtime with a fixup() hook, on a case by case basis. Other > thing to consider here, is the max_speed_hz supported by the PCB. For > example if the SPI flash supports 400 MHZ, the controller 200 MHZ, but > the PCB only 180 MHZ, then we'll have to synchronize all three. But this > seems like a discussion for other patch. The potential for board issues suggests that we should be taking the minimum of what the board DT and runtime discovery say - if the board limits things more than the parts we should assume that there's a system integration limitation there. > Let me know if you think that this patch is ok for now. I can update the > commit message if needed. It does work for now but it'd be nicer if we were doing this through recording the decision on the transfer.
On 12/10/20 5:33 PM, Mark Brown wrote: > On Thu, Dec 10, 2020 at 08:58:18AM +0000, Tudor.Ambarus@microchip.com wrote: >> On 12/9/20 10:30 PM, Serge Semin wrote: > >>>>>> Right, in general we aim to do this sort of fixup on the transfers >>>>>> and messages rather than the devices, I guess we might be missing >>>>>> validation in some of the flash acceleration paths or was this an issue >>>>>> seen through inspection? > >> We miss validation for the SPI controllers that provide the >> spi_controller_mem_ops with its exec_op() method. In this case the SPI >> core does not check if the max_speed_hz of spi_device overrides the >> max_speed_hz of controller. > >> This was seen through inspection. There are octal SPI NOR flashes that >> can run at 400 MHZ, and I've also seen SPI controllers that are limited >> to 200 MHZ (microchip's sama7g5 octal SPI for example, which is not yet >> in mainline). > > Right, that's the hole :/ > >>>> Ideally the driver wouldn't have to check though (no harm in doing so of >>>> course). > >> Right. I thought of doing this in the SPI core, rather than doing in (each) >> controller driver. > > Yes, we should just make sure things are OK in the core as much as we > can so there's less work for driver authors. > >>> If so then we'd need to have a dedicated speed-related field in the >>> spi_mem_op structure, which would be accordingly initialized by the >>> SPI-mem core. > >> We do need a max_speed_hz in the SPIMEM, but probably for other purposes: >> NOR flashes, for example, can discover the maximum supported frequency >> at run-time, when parsing the jesd216 SFDP tables. One may consider that >> the run-time discovered max_speed_hz should have a higher precedence than >> what we fill with the spi-max-frequency dt property, because the >> manufacturers/jesd216 standard know/s better. Of course, if the >> manufacturers put a wrong max_speed_hz in the sfdp table, that can be >> updated at runtime with a fixup() hook, on a case by case basis. Other >> thing to consider here, is the max_speed_hz supported by the PCB. For >> example if the SPI flash supports 400 MHZ, the controller 200 MHZ, but >> the PCB only 180 MHZ, then we'll have to synchronize all three. But this >> seems like a discussion for other patch. > > The potential for board issues suggests that we should be taking the > minimum of what the board DT and runtime discovery say - if the board > limits things more than the parts we should assume that there's a system > integration limitation there. > >> Let me know if you think that this patch is ok for now. I can update the >> commit message if needed. > > It does work for now but it'd be nicer if we were doing this through > recording the decision on the transfer. > Ok, we can drop the patch, as nobody complained about this until now. I can work on a better approach. Are you saying that we should calibrate/adjust the maximum supported frequency on each operation/command? Most of the commands can work at the same frequency. I know an exception: on SPI NOR flashes, the jesd216 standard specifies that the READ SFDP command should be run at 50MHz, even though the rest of the commands/opcodes can run at a higher frequency. It is common that flashes can run this command at 50+ MHz, and nobody bothered about adjusting the frequency at run-time until now. That being said, maybe we can calibrate/adjust a generic max frequency for most of the commands and treat the exceptions on a per operation basis. Cheers, ta
On Thu, Dec 10, 2020 at 04:32:02PM +0000, Tudor.Ambarus@microchip.com wrote: > On 12/10/20 5:33 PM, Mark Brown wrote: > > It does work for now but it'd be nicer if we were doing this through > > recording the decision on the transfer. > Ok, we can drop the patch, as nobody complained about this until now. I can TBH I've actually got it queued to apply and test, we can make things better incrementally but it seems fine for now and it's backportable if someone does run into trouble. > work on a better approach. Are you saying that we should calibrate/adjust the > maximum supported frequency on each operation/command? Most of the commands > can work at the same frequency. I know an exception: on SPI NOR flashes, the It's the way that everything else works. It's a bit inefficent though. > jesd216 standard specifies that the READ SFDP command should be run at 50MHz, > even though the rest of the commands/opcodes can run at a higher frequency. > It is common that flashes can run this command at 50+ MHz, and nobody bothered > about adjusting the frequency at run-time until now. That being said, maybe we > can calibrate/adjust a generic max frequency for most of the commands and > treat the exceptions on a per operation basis. If the spec says 50MHz for that one op we should probably stick to that by default - the chances of it making a difference seem low but better to stay in spec if we can.
On Wed, 9 Dec 2020 19:35:14 +0200, Tudor Ambarus wrote: > Make sure the max_speed_hz of spi_device does not override > the max_speed_hz of controller. Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: Limit the spi device max speed to controller's max speed commit: 9326e4f1e5dd1a4410c429638d3c412b6fc17040 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
Hi Mark, Tudor, On Fri, Dec 11, 2020 at 8:02 PM Mark Brown <broonie@kernel.org> wrote: > On Wed, 9 Dec 2020 19:35:14 +0200, Tudor Ambarus wrote: > > Make sure the max_speed_hz of spi_device does not override > > the max_speed_hz of controller. > > Applied to > > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next > > Thanks! > > [1/1] spi: Limit the spi device max speed to controller's max speed > commit: 9326e4f1e5dd1a4410c429638d3c412b6fc17040 > - if (!spi->max_speed_hz) > + if (!spi->max_speed_hz || > + spi->max_speed_hz > spi->controller->max_speed_hz) > spi->max_speed_hz = spi->controller->max_speed_hz; If spi->controller->max_speed_hz is zero, a non-zero spi->max_speed_hz will be overwritten by zero. Hence this broke spi-sh-msiof, which has the following check in sh_msiof_spi_set_clk_regs(): if (!spi_hz || !parent_rate) { WARN(1, "Invalid clock rate parameters %lu and %u\n", parent_rate, spi_hz); return; } Without this, the driver would trigger a division-by-zero later... Arguably all SPI controller drivers should fill in spi_controller.{min,max}_speed_hz, but as long as that is not the case, I think this patch should be reverted, or the check should be enhanced to make sure spi->controller->max_speed_hz is non-zero. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 12/15/20 4:24 PM, Geert Uytterhoeven wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Mark, Tudor, Hi, Geert, > > On Fri, Dec 11, 2020 at 8:02 PM Mark Brown <broonie@kernel.org> wrote: >> On Wed, 9 Dec 2020 19:35:14 +0200, Tudor Ambarus wrote: >>> Make sure the max_speed_hz of spi_device does not override >>> the max_speed_hz of controller. >> >> Applied to >> >> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next >> >> Thanks! >> >> [1/1] spi: Limit the spi device max speed to controller's max speed >> commit: 9326e4f1e5dd1a4410c429638d3c412b6fc17040 > >> - if (!spi->max_speed_hz) >> + if (!spi->max_speed_hz || >> + spi->max_speed_hz > spi->controller->max_speed_hz) >> spi->max_speed_hz = spi->controller->max_speed_hz; > > If spi->controller->max_speed_hz is zero, a non-zero spi->max_speed_hz > will be overwritten by zero. > oh, yes, you're right. > Hence this broke spi-sh-msiof, which has the following check in > sh_msiof_spi_set_clk_regs(): > > if (!spi_hz || !parent_rate) { > WARN(1, "Invalid clock rate parameters %lu and %u\n", > parent_rate, spi_hz); > return; > } > > Without this, the driver would trigger a division-by-zero later... > > Arguably all SPI controller drivers should fill in > spi_controller.{min,max}_speed_hz, but as long as that is not the case, > I think this patch should be reverted, or the check should be enhanced > to make sure spi->controller->max_speed_hz is non-zero. > I'm fine both ways, since this should be improved anyway. Will send a patch with the non-zero check and add your Reported-by, Suggested-by tags, in case Mark decides to keep it. Cheers, ta > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds >
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index cd3c395b4e90..51d7c004fbab 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -3378,7 +3378,8 @@ int spi_setup(struct spi_device *spi) if (status) return status; - if (!spi->max_speed_hz) + if (!spi->max_speed_hz || + spi->max_speed_hz > spi->controller->max_speed_hz) spi->max_speed_hz = spi->controller->max_speed_hz; mutex_lock(&spi->controller->io_mutex);
Make sure the max_speed_hz of spi_device does not override the max_speed_hz of controller. Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- drivers/spi/spi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)