| Message ID | 20250207-dlech-mainline-spi-engine-offload-2-v8-1-e48a489be48c@baylibre.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8e02d188698851436f76038ea998b726193d1b10 |
| Headers | show |
| Series | spi: axi-spi-engine: add offload support | expand |
On Fri, Feb 07, 2025 at 02:08:58PM -0600, David Lechner wrote: > Add the basic infrastructure to support SPI offload providers and > consumers. > > SPI offloading is a feature that allows the SPI controller to perform > transfers without any CPU intervention. This is useful, e.g. for > high-speed data acquisition. > > SPI controllers with offload support need to implement the get_offload > and put_offload callbacks and can use the devm_spi_offload_alloc() to > allocate offload instances. > > SPI peripheral drivers will call devm_spi_offload_get() to get a > reference to the matching offload instance. This offload instance can > then be attached to a SPI message to request offloading that message. > > It is expected that SPI controllers with offload support will check for > the offload instance in the SPI message in the ctlr->optimize_message() > callback and handle it accordingly. > > CONFIG_SPI_OFFLOAD is intended to be a select-only option. Both > consumer and provider drivers should `select SPI_OFFLOAD` in their > Kconfig to ensure that the SPI core is built with offload support. (I know that this is now in SPI tree, but still we have time to address something) > +++ b/include/linux/spi/offload/consumer.h > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (C) 2024 Analog Devices Inc. > + * Copyright (C) 2024 BayLibre, SAS > + */ > + > +#ifndef __LINUX_SPI_OFFLOAD_CONSUMER_H > +#define __LINUX_SPI_OFFLOAD_CONSUMER_H > + > +#include <linux/module.h> > +#include <linux/spi/offload/types.h> > +#include <linux/types.h> > +MODULE_IMPORT_NS("SPI_OFFLOAD"); This diminishes the point of the namespaces. Anybody who includes a (dangling) header gets namespace imported, which is not good. Same for other globally visible headers. (This is the main concern of this patch). ... > +#ifndef __LINUX_SPI_OFFLOAD_TYPES_H > +#define __LINUX_SPI_OFFLOAD_TYPES_H + linux/bits.h > +#include <linux/types.h> > + > +struct device; > + > +/* Offload can be triggered by external hardware event. */ > +#define SPI_OFFLOAD_CAP_TRIGGER BIT(0) > +/* Offload can record and then play back TX data when triggered. */ > +#define SPI_OFFLOAD_CAP_TX_STATIC_DATA BIT(1) > +/* Offload can get TX data from an external stream source. */ > +#define SPI_OFFLOAD_CAP_TX_STREAM_DMA BIT(2) > +/* Offload can send RX data to an external stream sink. */ > +#define SPI_OFFLOAD_CAP_RX_STREAM_DMA BIT(3) > +/** > + * struct spi_offload_config - offload configuration > + * > + * This is used to request an offload with specific configuration. > + */ > +struct spi_offload_config { > + /** @capability_flags: required capabilities. See %SPI_OFFLOAD_CAP_* */ > + u32 capability_flags; > +}; > + > +/** > + * struct spi_offload - offload instance > + */ > +struct spi_offload { > + /** @provider_dev: for get/put reference counting */ > + struct device *provider_dev; > + /** @priv: provider driver private data */ > + void *priv; > +}; > + > +#endif /* __LINUX_SPI_OFFLOAD_TYPES_H */
On 2/10/25 10:45 AM, Andy Shevchenko wrote: > On Fri, Feb 07, 2025 at 02:08:58PM -0600, David Lechner wrote: >> Add the basic infrastructure to support SPI offload providers and >> consumers. >> >> SPI offloading is a feature that allows the SPI controller to perform >> transfers without any CPU intervention. This is useful, e.g. for >> high-speed data acquisition. >> >> SPI controllers with offload support need to implement the get_offload >> and put_offload callbacks and can use the devm_spi_offload_alloc() to >> allocate offload instances. >> >> SPI peripheral drivers will call devm_spi_offload_get() to get a >> reference to the matching offload instance. This offload instance can >> then be attached to a SPI message to request offloading that message. >> >> It is expected that SPI controllers with offload support will check for >> the offload instance in the SPI message in the ctlr->optimize_message() >> callback and handle it accordingly. >> >> CONFIG_SPI_OFFLOAD is intended to be a select-only option. Both >> consumer and provider drivers should `select SPI_OFFLOAD` in their >> Kconfig to ensure that the SPI core is built with offload support. > > (I know that this is now in SPI tree, but still we have time to address something) > >> +++ b/include/linux/spi/offload/consumer.h > >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Copyright (C) 2024 Analog Devices Inc. >> + * Copyright (C) 2024 BayLibre, SAS >> + */ >> + >> +#ifndef __LINUX_SPI_OFFLOAD_CONSUMER_H >> +#define __LINUX_SPI_OFFLOAD_CONSUMER_H >> + >> +#include <linux/module.h> >> +#include <linux/spi/offload/types.h> >> +#include <linux/types.h> > >> +MODULE_IMPORT_NS("SPI_OFFLOAD"); > > This diminishes the point of the namespaces. Anybody who includes a (dangling) > header gets namespace imported, which is not good. Same for other globally > visible headers. > > (This is the main concern of this patch). In this case, we specifically split up the headers so that the only time you would ever include this header is if you need to call functions in this namespace (i.e. struct definitions are in linux/spi/offload/types.h which doesn't import the namespace). So this doesn't actually seem like a problem to me.
On Mon, Feb 10, 2025 at 11:11:23AM -0600, David Lechner wrote: > On 2/10/25 10:45 AM, Andy Shevchenko wrote: > > On Fri, Feb 07, 2025 at 02:08:58PM -0600, David Lechner wrote: > >> +MODULE_IMPORT_NS("SPI_OFFLOAD"); > > This diminishes the point of the namespaces. Anybody who includes a (dangling) > > header gets namespace imported, which is not good. Same for other globally > > visible headers. > In this case, we specifically split up the headers so that the only time you > would ever include this header is if you need to call functions in this > namespace (i.e. struct definitions are in linux/spi/offload/types.h which > doesn't import the namespace). So this doesn't actually seem like a problem > to me. Indeed - I can't see any case where a user would need the header without needing the namespace.
On Mon, Feb 10, 2025 at 05:48:00PM +0000, Mark Brown wrote: > On Mon, Feb 10, 2025 at 11:11:23AM -0600, David Lechner wrote: > > On 2/10/25 10:45 AM, Andy Shevchenko wrote: > > > On Fri, Feb 07, 2025 at 02:08:58PM -0600, David Lechner wrote: > > > >> +MODULE_IMPORT_NS("SPI_OFFLOAD"); > > > > This diminishes the point of the namespaces. Anybody who includes a (dangling) > > > header gets namespace imported, which is not good. Same for other globally > > > visible headers. > > > In this case, we specifically split up the headers so that the only time you > > would ever include this header is if you need to call functions in this > > namespace (i.e. struct definitions are in linux/spi/offload/types.h which > > doesn't import the namespace). So this doesn't actually seem like a problem > > to me. > > Indeed - I can't see any case where a user would need the header without > needing the namespace. You are looking from the other end. What I'm telling is that anyone who adds a header, automatically gets a namespace. What's the point to have namespace if it won't easily prevent from (ab)using it in the code. I consider putting MODULE_IMPORT_NS() in the headers a bit weird.
On Mon, Feb 10, 2025 at 10:33:31PM +0200, Andy Shevchenko wrote: > On Mon, Feb 10, 2025 at 05:48:00PM +0000, Mark Brown wrote: > > On Mon, Feb 10, 2025 at 11:11:23AM -0600, David Lechner wrote: > > > In this case, we specifically split up the headers so that the only time you > > > would ever include this header is if you need to call functions in this > > > namespace (i.e. struct definitions are in linux/spi/offload/types.h which > > > doesn't import the namespace). So this doesn't actually seem like a problem > > > to me. > > Indeed - I can't see any case where a user would need the header without > > needing the namespace. > You are looking from the other end. What I'm telling is that anyone who adds > a header, automatically gets a namespace. What's the point to have namespace > if it won't easily prevent from (ab)using it in the code. I consider putting > MODULE_IMPORT_NS() in the headers a bit weird. Sure, but there's no case where anyone should ever be adding the header without adding the namespace which does rather sound like the sort of thing where you should just move the namespace addition to the header.
On Tue, Feb 11, 2025 at 01:00:08PM +0000, Mark Brown wrote: > On Mon, Feb 10, 2025 at 10:33:31PM +0200, Andy Shevchenko wrote: > > On Mon, Feb 10, 2025 at 05:48:00PM +0000, Mark Brown wrote: > > > On Mon, Feb 10, 2025 at 11:11:23AM -0600, David Lechner wrote: > > > > > In this case, we specifically split up the headers so that the only time you > > > > would ever include this header is if you need to call functions in this > > > > namespace (i.e. struct definitions are in linux/spi/offload/types.h which > > > > doesn't import the namespace). So this doesn't actually seem like a problem > > > > to me. > > > > Indeed - I can't see any case where a user would need the header without > > > needing the namespace. > > > You are looking from the other end. What I'm telling is that anyone who adds > > a header, automatically gets a namespace. What's the point to have namespace > > if it won't easily prevent from (ab)using it in the code. I consider putting > > MODULE_IMPORT_NS() in the headers a bit weird. > > Sure, but there's no case where anyone should ever be adding the header > without adding the namespace which does rather sound like the sort of > thing where you should just move the namespace addition to the header. $ git grep -lw MODULE_IMPORT_NS | wc -l 651 $ git grep -lw MODULE_IMPORT_NS | grep '\.h$' drivers/base/firmware_loader/sysfs.h drivers/iio/adc/ltc2497.h drivers/pwm/pwm-dwc.h ^^^ These ones are probably fine as they are not in include/ include/kunit/visibility.h include/linux/module.h include/linux/pwm.h I believe these three are misuses of MODULE_IMPORT_NS(). Because one may add a header just as a "proxy" one (copy'n'paste, for example) and we know that is real as we saw a lot of code that has semi-random header inclusion blocks.
On Tue, Feb 11, 2025 at 04:20:50PM +0200, Andy Shevchenko wrote: > On Tue, Feb 11, 2025 at 01:00:08PM +0000, Mark Brown wrote: > > On Mon, Feb 10, 2025 at 10:33:31PM +0200, Andy Shevchenko wrote: > > > On Mon, Feb 10, 2025 at 05:48:00PM +0000, Mark Brown wrote: > > > > On Mon, Feb 10, 2025 at 11:11:23AM -0600, David Lechner wrote: > > > > > > > In this case, we specifically split up the headers so that the only time you > > > > > would ever include this header is if you need to call functions in this > > > > > namespace (i.e. struct definitions are in linux/spi/offload/types.h which > > > > > doesn't import the namespace). So this doesn't actually seem like a problem > > > > > to me. > > > > > > Indeed - I can't see any case where a user would need the header without > > > > needing the namespace. > > > > > You are looking from the other end. What I'm telling is that anyone who adds > > > a header, automatically gets a namespace. What's the point to have namespace > > > if it won't easily prevent from (ab)using it in the code. I consider putting > > > MODULE_IMPORT_NS() in the headers a bit weird. > > > > Sure, but there's no case where anyone should ever be adding the header > > without adding the namespace which does rather sound like the sort of > > thing where you should just move the namespace addition to the header. > > $ git grep -lw MODULE_IMPORT_NS | wc -l > 651 > > $ git grep -lw MODULE_IMPORT_NS | grep '\.h$' > > drivers/base/firmware_loader/sysfs.h > drivers/iio/adc/ltc2497.h > drivers/pwm/pwm-dwc.h > ^^^ These ones are probably fine as they are not in include/ > > include/kunit/visibility.h > include/linux/module.h > include/linux/pwm.h > > I believe these three are misuses of MODULE_IMPORT_NS(). Because one may add _Ttwo_, of course, module.h provides the macro :-) > a header just as a "proxy" one (copy'n'paste, for example) and we know that is > real as we saw a lot of code that has semi-random header inclusion blocks.
On Tue, Feb 11, 2025 at 04:29:33PM +0200, Andy Shevchenko wrote: > On Tue, Feb 11, 2025 at 04:20:50PM +0200, Andy Shevchenko wrote: > > On Tue, Feb 11, 2025 at 01:00:08PM +0000, Mark Brown wrote: > > > On Mon, Feb 10, 2025 at 10:33:31PM +0200, Andy Shevchenko wrote: > > > > On Mon, Feb 10, 2025 at 05:48:00PM +0000, Mark Brown wrote: > > > > > On Mon, Feb 10, 2025 at 11:11:23AM -0600, David Lechner wrote: > > > > > > > > > In this case, we specifically split up the headers so that the only time you > > > > > > would ever include this header is if you need to call functions in this > > > > > > namespace (i.e. struct definitions are in linux/spi/offload/types.h which > > > > > > doesn't import the namespace). So this doesn't actually seem like a problem > > > > > > to me. > > > > > > > > Indeed - I can't see any case where a user would need the header without > > > > > needing the namespace. > > > > > > > You are looking from the other end. What I'm telling is that anyone who adds > > > > a header, automatically gets a namespace. What's the point to have namespace > > > > if it won't easily prevent from (ab)using it in the code. I consider putting > > > > MODULE_IMPORT_NS() in the headers a bit weird. > > > > > > Sure, but there's no case where anyone should ever be adding the header > > > without adding the namespace which does rather sound like the sort of > > > thing where you should just move the namespace addition to the header. > > > > $ git grep -lw MODULE_IMPORT_NS | wc -l > > 651 > > > > $ git grep -lw MODULE_IMPORT_NS | grep '\.h$' > > > > drivers/base/firmware_loader/sysfs.h > > drivers/iio/adc/ltc2497.h > > drivers/pwm/pwm-dwc.h > > ^^^ These ones are probably fine as they are not in include/ > > > > include/kunit/visibility.h > > include/linux/module.h > > include/linux/pwm.h > > > > I believe these three are misuses of MODULE_IMPORT_NS(). Because one may add > > _Two_, of course, module.h provides the macro :-) And after looking into include/kunit/visibility.h it becomes only a single one. So, PWM is abuser of MODULE_IMPORT_NS() and this series added one more. > > a header just as a "proxy" one (copy'n'paste, for example) and we know that is > > real as we saw a lot of code that has semi-random header inclusion blocks.
On Tue, Feb 11, 2025 at 04:31:45PM +0200, Andy Shevchenko wrote: > On Tue, Feb 11, 2025 at 04:29:33PM +0200, Andy Shevchenko wrote: > > On Tue, Feb 11, 2025 at 04:20:50PM +0200, Andy Shevchenko wrote: > > > On Tue, Feb 11, 2025 at 01:00:08PM +0000, Mark Brown wrote: > > > > On Mon, Feb 10, 2025 at 10:33:31PM +0200, Andy Shevchenko wrote: > > > > > On Mon, Feb 10, 2025 at 05:48:00PM +0000, Mark Brown wrote: > > > > > > On Mon, Feb 10, 2025 at 11:11:23AM -0600, David Lechner wrote: > > > > > > > > > > > In this case, we specifically split up the headers so that the only time you > > > > > > > would ever include this header is if you need to call functions in this > > > > > > > namespace (i.e. struct definitions are in linux/spi/offload/types.h which > > > > > > > doesn't import the namespace). So this doesn't actually seem like a problem > > > > > > > to me. > > > > > > > > > > Indeed - I can't see any case where a user would need the header without > > > > > > needing the namespace. > > > > > > > > > You are looking from the other end. What I'm telling is that anyone who adds > > > > > a header, automatically gets a namespace. What's the point to have namespace > > > > > if it won't easily prevent from (ab)using it in the code. I consider putting > > > > > MODULE_IMPORT_NS() in the headers a bit weird. > > > > > > > > Sure, but there's no case where anyone should ever be adding the header > > > > without adding the namespace which does rather sound like the sort of > > > > thing where you should just move the namespace addition to the header. > > > > > > $ git grep -lw MODULE_IMPORT_NS | wc -l > > > 651 > > > > > > $ git grep -lw MODULE_IMPORT_NS | grep '\.h$' > > > > > > drivers/base/firmware_loader/sysfs.h > > > drivers/iio/adc/ltc2497.h > > > drivers/pwm/pwm-dwc.h > > > ^^^ These ones are probably fine as they are not in include/ > > > > > > include/kunit/visibility.h > > > include/linux/module.h > > > include/linux/pwm.h > > > > > > I believe these three are misuses of MODULE_IMPORT_NS(). Because one may add > > > > _Two_, of course, module.h provides the macro :-) > > And after looking into include/kunit/visibility.h it becomes only a single one. > So, PWM is abuser of MODULE_IMPORT_NS() and this series added one more. > > > a header just as a "proxy" one (copy'n'paste, for example) and we know that is > > > real as we saw a lot of code that has semi-random header inclusion blocks. And thinking of more realistic example when we want header and do *not* want a namespace is the simple use of the macro / or data type from it without actually relying on the APIs. So, in case of the header structure like foo_constants.h foo_types.h foo_api.h foo_uplevel_something.h The MODULE_IMPORT_NS() would make sense only to foo_api.h. And I still would question that. As I explained that header may simply become a stale one or being used by a mistake.
Hello Andy, On Tue, Feb 11, 2025 at 04:35:34PM +0200, Andy Shevchenko wrote: > On Tue, Feb 11, 2025 at 04:31:45PM +0200, Andy Shevchenko wrote: > > On Tue, Feb 11, 2025 at 04:29:33PM +0200, Andy Shevchenko wrote: > > > On Tue, Feb 11, 2025 at 04:20:50PM +0200, Andy Shevchenko wrote: > > > > On Tue, Feb 11, 2025 at 01:00:08PM +0000, Mark Brown wrote: > > > > > On Mon, Feb 10, 2025 at 10:33:31PM +0200, Andy Shevchenko wrote: > > > > > > On Mon, Feb 10, 2025 at 05:48:00PM +0000, Mark Brown wrote: > > > > > > > On Mon, Feb 10, 2025 at 11:11:23AM -0600, David Lechner wrote: > > > > > > > > > > > > > In this case, we specifically split up the headers so that the only time you > > > > > > > > would ever include this header is if you need to call functions in this > > > > > > > > namespace (i.e. struct definitions are in linux/spi/offload/types.h which > > > > > > > > doesn't import the namespace). So this doesn't actually seem like a problem > > > > > > > > to me. > > > > > > > > > > > > Indeed - I can't see any case where a user would need the header without > > > > > > > needing the namespace. > > > > > > > > > > > You are looking from the other end. What I'm telling is that anyone who adds > > > > > > a header, automatically gets a namespace. What's the point to have namespace > > > > > > if it won't easily prevent from (ab)using it in the code. I consider putting > > > > > > MODULE_IMPORT_NS() in the headers a bit weird. There was a similar discussion some time ago about the lpss pwm driver (https://lore.kernel.org/linux-pwm/Z09YJGifvpENYNPy@smile.fi.intel.com/). The arguments that you didn't accept back then already are similar to the ones that were brought forward here. The TL;DR; is: Adding MODULE_IMPORT_NS() to a header makes it easier for code to use the exported symbols. Yes, that includes abusers of the code. But if you mostly care about the regular users of an API/ABI, making things easy for those is the thing that matters. Agreed, if you think that module namespaces are primarily a line of defence against abusers, adding the import to the header weakens that defence (a bit). However a typical header includes function prototypes and macros. Those also make it easier for abusers. With your argumentation we better don't create headers at all? There are other benefits of module namespaces like reducing the set of globally available symbols which speeds up module loading or the ability to see in the module meta data that a namespace is used. > > > > > Sure, but there's no case where anyone should ever be adding the header > > > > > without adding the namespace which does rather sound like the sort of > > > > > thing where you should just move the namespace addition to the header. > > > > > > > > $ git grep -lw MODULE_IMPORT_NS | wc -l > > > > 651 > > > > > > > > $ git grep -lw MODULE_IMPORT_NS | grep '\.h$' > > > > > > > > drivers/base/firmware_loader/sysfs.h > > > > drivers/iio/adc/ltc2497.h > > > > drivers/pwm/pwm-dwc.h > > > > ^^^ These ones are probably fine as they are not in include/ > > > > > > > > include/kunit/visibility.h > > > > include/linux/module.h > > > > include/linux/pwm.h > > > > > > > > I believe these three are misuses of MODULE_IMPORT_NS(). Because one may add > > > > > > _Two_, of course, module.h provides the macro :-) > > > > And after looking into include/kunit/visibility.h it becomes only a single one. > > So, PWM is abuser of MODULE_IMPORT_NS() and this series added one more. > > > > > a header just as a "proxy" one (copy'n'paste, for example) and we know that is > > > > real as we saw a lot of code that has semi-random header inclusion blocks. > > And thinking of more realistic example when we want header and do *not* want a > namespace is the simple use of the macro / or data type from it without > actually relying on the APIs. The problem of your more realistic example is that it doesn't apply here. A user of include/linux/pwm.h (or the header under discussion here) won't only use a macro or two and so not benefit from the imported module namespace. Nobody intends to import all possible namespaces in <linux/kernel.h>. > So, in case of the header structure like > > foo_constants.h > foo_types.h > foo_api.h > foo_uplevel_something.h > > The MODULE_IMPORT_NS() would make sense only to foo_api.h. And I still would > question that. As I explained that header may simply become a stale one or > being used by a mistake. I have no problem here. If the header becomes stale we will most probably notice that eventually and remove it. Maybe the unused namespace even makes it easier to spot that issue. See https://lore.kernel.org/r/20250123103939.357160-2-u.kleine-koenig@baylibre.com for an example which I found exactly like that. Best regards Uwe
On Tue, Feb 11, 2025 at 07:45:30PM +0100, Uwe Kleine-König wrote: > There was a similar discussion some time ago about the lpss pwm driver > (https://lore.kernel.org/linux-pwm/Z09YJGifvpENYNPy@smile.fi.intel.com/). > The arguments that you didn't accept back then already are similar to > the ones that were brought forward here. > The TL;DR; is: Adding MODULE_IMPORT_NS() to a header makes it easier for > code to use the exported symbols. Yes, that includes abusers of the > code. > But if you mostly care about the regular users of an API/ABI, making > things easy for those is the thing that matters. Agreed, if you think > that module namespaces are primarily a line of defence against abusers, > adding the import to the header weakens that defence (a bit). However a > typical header includes function prototypes and macros. Those also make > it easier for abusers. With your argumentation we better don't create > headers at all? > There are other benefits of module namespaces like reducing the set of > globally available symbols which speeds up module loading or the > ability to see in the module meta data that a namespace is used. FWIW I fully endorse what Uwe is saying here, forcing every user of the API to separately import the symbols seems more likely to create busywork than to avoid problems.
On Tue, Feb 11, 2025 at 07:45:30PM +0100, Uwe Kleine-König wrote: > Hello Andy, > > On Tue, Feb 11, 2025 at 04:35:34PM +0200, Andy Shevchenko wrote: > > On Tue, Feb 11, 2025 at 04:31:45PM +0200, Andy Shevchenko wrote: > > > On Tue, Feb 11, 2025 at 04:29:33PM +0200, Andy Shevchenko wrote: > > > > On Tue, Feb 11, 2025 at 04:20:50PM +0200, Andy Shevchenko wrote: > > > > > On Tue, Feb 11, 2025 at 01:00:08PM +0000, Mark Brown wrote: > > > > > > On Mon, Feb 10, 2025 at 10:33:31PM +0200, Andy Shevchenko wrote: > > > > > > > On Mon, Feb 10, 2025 at 05:48:00PM +0000, Mark Brown wrote: > > > > > > > > On Mon, Feb 10, 2025 at 11:11:23AM -0600, David Lechner wrote: > > > > > > > > > > > > > > > In this case, we specifically split up the headers so that the only time you > > > > > > > > > would ever include this header is if you need to call functions in this > > > > > > > > > namespace (i.e. struct definitions are in linux/spi/offload/types.h which > > > > > > > > > doesn't import the namespace). So this doesn't actually seem like a problem > > > > > > > > > to me. > > > > > > > > > > > > > > Indeed - I can't see any case where a user would need the header without > > > > > > > > needing the namespace. > > > > > > > > > > > > > You are looking from the other end. What I'm telling is that anyone who adds > > > > > > > a header, automatically gets a namespace. What's the point to have namespace > > > > > > > if it won't easily prevent from (ab)using it in the code. I consider putting > > > > > > > MODULE_IMPORT_NS() in the headers a bit weird. > > There was a similar discussion some time ago about the lpss pwm driver > (https://lore.kernel.org/linux-pwm/Z09YJGifvpENYNPy@smile.fi.intel.com/). > The arguments that you didn't accept back then already are similar to > the ones that were brought forward here. > The TL;DR; is: Adding MODULE_IMPORT_NS() to a header makes it easier for > code to use the exported symbols. Yes, that includes abusers of the > code. > > But if you mostly care about the regular users of an API/ABI, making > things easy for those is the thing that matters. Agreed, if you think > that module namespaces are primarily a line of defence against abusers, > adding the import to the header weakens that defence (a bit). However a > typical header includes function prototypes and macros. Those also make > it easier for abusers. With your argumentation we better don't create > headers at all? > > There are other benefits of module namespaces like reducing the set of > globally available symbols which speeds up module loading or the > ability to see in the module meta data that a namespace is used. Thank you for summarizing the previous discussion. > > > > > > Sure, but there's no case where anyone should ever be adding the header > > > > > > without adding the namespace which does rather sound like the sort of > > > > > > thing where you should just move the namespace addition to the header. > > > > > > > > > > $ git grep -lw MODULE_IMPORT_NS | wc -l > > > > > 651 > > > > > > > > > > $ git grep -lw MODULE_IMPORT_NS | grep '\.h$' > > > > > > > > > > drivers/base/firmware_loader/sysfs.h > > > > > drivers/iio/adc/ltc2497.h > > > > > drivers/pwm/pwm-dwc.h > > > > > ^^^ These ones are probably fine as they are not in include/ > > > > > > > > > > include/kunit/visibility.h > > > > > include/linux/module.h > > > > > include/linux/pwm.h > > > > > > > > > > I believe these three are misuses of MODULE_IMPORT_NS(). Because one may add > > > > > > > > _Two_, of course, module.h provides the macro :-) > > > > > > And after looking into include/kunit/visibility.h it becomes only a single one. > > > So, PWM is abuser of MODULE_IMPORT_NS() and this series added one more. > > > > > > > a header just as a "proxy" one (copy'n'paste, for example) and we know that is > > > > > real as we saw a lot of code that has semi-random header inclusion blocks. > > > > And thinking of more realistic example when we want header and do *not* want a > > namespace is the simple use of the macro / or data type from it without > > actually relying on the APIs. > > The problem of your more realistic example is that it doesn't apply > here. A user of include/linux/pwm.h (or the header under discussion > here) won't only use a macro or two and so not benefit from the imported > module namespace. It may not apply _today_, but it may be applicable tomorrow as headers are tend to grow and use another headers and so on. > Nobody intends to import all possible namespaces in <linux/kernel.h>. > > > So, in case of the header structure like > > > > foo_constants.h > > foo_types.h > > foo_api.h > > foo_uplevel_something.h > > > > The MODULE_IMPORT_NS() would make sense only to foo_api.h. And I still would > > question that. As I explained that header may simply become a stale one or > > being used by a mistake. > > I have no problem here. If the header becomes stale we will most > probably notice that eventually and remove it. Lol. Look at the header hell we have now. 98% code in the drivers/ just show that the developers either don't care or do not understand C (in terms of what headers are for and why it's important to follow IWYU principle). > Maybe the unused namespace even makes it easier to spot that issue. Do we have an existing tools for that? > See > https://lore.kernel.org/r/20250123103939.357160-2-u.kleine-koenig@baylibre.com > for an example which I found exactly like that.
On Tue, Feb 11, 2025 at 06:53:17PM +0000, Mark Brown wrote: > On Tue, Feb 11, 2025 at 07:45:30PM +0100, Uwe Kleine-König wrote: > > > There was a similar discussion some time ago about the lpss pwm driver > > (https://lore.kernel.org/linux-pwm/Z09YJGifvpENYNPy@smile.fi.intel.com/). > > The arguments that you didn't accept back then already are similar to > > the ones that were brought forward here. > > The TL;DR; is: Adding MODULE_IMPORT_NS() to a header makes it easier for > > code to use the exported symbols. Yes, that includes abusers of the > > code. > > > But if you mostly care about the regular users of an API/ABI, making > > things easy for those is the thing that matters. Agreed, if you think > > that module namespaces are primarily a line of defence against abusers, > > adding the import to the header weakens that defence (a bit). However a > > typical header includes function prototypes and macros. Those also make > > it easier for abusers. With your argumentation we better don't create > > headers at all? > > > There are other benefits of module namespaces like reducing the set of > > globally available symbols which speeds up module loading or the > > ability to see in the module meta data that a namespace is used. > > FWIW I fully endorse what Uwe is saying here, forcing every user of the > API to separately import the symbols seems more likely to create > busywork than to avoid problems. I see. Another problem that comes to my mind just now is the module.h to be included by every header that wants to use MODULE_*() macro. Maybe someone wants to split mod_namespace.h to decrease an added chaos?
Hello Andy, On Tue, Feb 11, 2025 at 09:12:30PM +0200, Andy Shevchenko wrote: > On Tue, Feb 11, 2025 at 07:45:30PM +0100, Uwe Kleine-König wrote: > > I have no problem here. If the header becomes stale we will most > > probably notice that eventually and remove it. > > Lol. Look at the header hell we have now. 98% code in the drivers/ just show > that the developers either don't care or do not understand C (in terms of > what headers are for and why it's important to follow IWYU principle). Yeah, there is a problem. The source is that we have a metric ton of recursive includes (i.e. headers that include other headers that include still more headers). Even if you care, its sometimes hard to know which headers you actually need. One idea on my long-term list is to add a machine-parsable info to header files about the list of symbols that the given file is responsible for. With that in place we could create a linter that tells you that this source file doesn't use any symbols from <linux/of_irq.h> and it should #include <linux/of.h> directly instead to make use of symbols defined there. > > Maybe the unused namespace even makes it easier to spot that issue. > > Do we have an existing tools for that? There is https://lore.kernel.org/all/20250123110951.370759-2-u.kleine-koenig@baylibre.com/ Best regards Uwe
On Wed, Feb 12, 2025 at 09:52:37AM +0100, Uwe Kleine-König wrote: > On Tue, Feb 11, 2025 at 09:12:30PM +0200, Andy Shevchenko wrote: > > On Tue, Feb 11, 2025 at 07:45:30PM +0100, Uwe Kleine-König wrote: > > > I have no problem here. If the header becomes stale we will most > > > probably notice that eventually and remove it. > > > > Lol. Look at the header hell we have now. 98% code in the drivers/ just show > > that the developers either don't care or do not understand C (in terms of > > what headers are for and why it's important to follow IWYU principle). > > Yeah, there is a problem. The source is that we have a metric ton of > recursive includes (i.e. headers that include other headers that include > still more headers). Even if you care, its sometimes hard to know which > headers you actually need. One idea on my long-term list is to add a > machine-parsable info to header files about the list of symbols that the > given file is responsible for. With that in place we could create a > linter that tells you that this source file doesn't use any symbols from > <linux/of_irq.h> and it should #include <linux/of.h> directly instead to > make use of symbols defined there. > > > > Maybe the unused namespace even makes it easier to spot that issue. > > > > Do we have an existing tools for that? > > There is https://lore.kernel.org/all/20250123110951.370759-2-u.kleine-koenig@baylibre.com/ But it was rejected. So, the answer is "we currently do not have tools".
On Wed, Feb 12, 2025 at 09:52:37AM +0100, Uwe Kleine-König wrote: > On Tue, Feb 11, 2025 at 09:12:30PM +0200, Andy Shevchenko wrote: > > On Tue, Feb 11, 2025 at 07:45:30PM +0100, Uwe Kleine-König wrote: > > > I have no problem here. If the header becomes stale we will most > > > probably notice that eventually and remove it. > > > > Lol. Look at the header hell we have now. 98% code in the drivers/ just show > > that the developers either don't care or do not understand C (in terms of > > what headers are for and why it's important to follow IWYU principle). > > Yeah, there is a problem. The source is that we have a metric ton of > recursive includes (i.e. headers that include other headers that include > still more headers). Even if you care, its sometimes hard to know which > headers you actually need. One idea on my long-term list is to add a > machine-parsable info to header files about the list of symbols that the > given file is responsible for. With that in place we could create a > linter that tells you that this source file doesn't use any symbols from > <linux/of_irq.h> and it should #include <linux/of.h> directly instead to > make use of symbols defined there. There were already few attempts to untangle the dependency hell we have in LK project, but all seems to fail. The infamous one by Ingo stale, however a few patches (out of more than 2200!) made upstream. So, any tooling for that will be warmly accepted!
diff --git a/MAINTAINERS b/MAINTAINERS index e4f5d8f6858170e63339aaa3380c3845cc08ab84..c37504e2e19f067b9835a28708dfd1d25700a608 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -22342,6 +22342,12 @@ F: Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml F: drivers/mtd/spi-nor/ F: include/linux/mtd/spi-nor.h +SPI OFFLOAD +R: David Lechner <dlechner@baylibre.com> +F: drivers/spi/spi-offload.c +F: include/linux/spi/spi-offload.h +K: spi_offload + SPI SUBSYSTEM M: Mark Brown <broonie@kernel.org> L: linux-spi@vger.kernel.org diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index ea8a310329274bb2701e265cd152a56fb4e0f3a7..02064a4e292815ec0213e2e446b4f90ed8855a52 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -55,6 +55,9 @@ config SPI_MEM This extension is meant to simplify interaction with SPI memories by providing a high-level interface to send memory-like commands. +config SPI_OFFLOAD + bool + comment "SPI Master Controller Drivers" config SPI_AIROHA_SNFI diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index 9db7554c1864bf9b37dcf59c16eb76f5af03a7e8..bb5fc20df21332232533c2e70c0cc230f6bcf27f 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -10,6 +10,7 @@ ccflags-$(CONFIG_SPI_DEBUG) := -DDEBUG obj-$(CONFIG_SPI_MASTER) += spi.o obj-$(CONFIG_SPI_MEM) += spi-mem.o obj-$(CONFIG_SPI_MUX) += spi-mux.o +obj-$(CONFIG_SPI_OFFLOAD) += spi-offload.o obj-$(CONFIG_SPI_SPIDEV) += spidev.o obj-$(CONFIG_SPI_LOOPBACK_TEST) += spi-loopback-test.o diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c new file mode 100644 index 0000000000000000000000000000000000000000..3a40ef30debf09c6fd7b2c14526f3e5976e2b21f --- /dev/null +++ b/drivers/spi/spi-offload.c @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2024 Analog Devices Inc. + * Copyright (C) 2024 BayLibre, SAS + */ + +/* + * SPI Offloading support. + * + * Some SPI controllers support offloading of SPI transfers. Essentially, this + * is the ability for a SPI controller to perform SPI transfers with minimal + * or even no CPU intervention, e.g. via a specialized SPI controller with a + * hardware trigger or via a conventional SPI controller using a non-Linux MCU + * processor core to offload the work. + */ + +#define DEFAULT_SYMBOL_NAMESPACE "SPI_OFFLOAD" + +#include <linux/cleanup.h> +#include <linux/device.h> +#include <linux/export.h> +#include <linux/mutex.h> +#include <linux/spi/offload/consumer.h> +#include <linux/spi/offload/provider.h> +#include <linux/spi/offload/types.h> +#include <linux/spi/spi.h> +#include <linux/types.h> + +struct spi_controller_and_offload { + struct spi_controller *controller; + struct spi_offload *offload; +}; + +/** + * devm_spi_offload_alloc() - Allocate offload instance + * @dev: Device for devm purposes and assigned to &struct spi_offload.provider_dev + * @priv_size: Size of private data to allocate + * + * Offload providers should use this to allocate offload instances. + * + * Return: Pointer to new offload instance or error on failure. + */ +struct spi_offload *devm_spi_offload_alloc(struct device *dev, + size_t priv_size) +{ + struct spi_offload *offload; + void *priv; + + offload = devm_kzalloc(dev, sizeof(*offload), GFP_KERNEL); + if (!offload) + return ERR_PTR(-ENOMEM); + + priv = devm_kzalloc(dev, priv_size, GFP_KERNEL); + if (!priv) + return ERR_PTR(-ENOMEM); + + offload->provider_dev = dev; + offload->priv = priv; + + return offload; +} +EXPORT_SYMBOL_GPL(devm_spi_offload_alloc); + +static void spi_offload_put(void *data) +{ + struct spi_controller_and_offload *resource = data; + + resource->controller->put_offload(resource->offload); + kfree(resource); +} + +/** + * devm_spi_offload_get() - Get an offload instance + * @dev: Device for devm purposes + * @spi: SPI device to use for the transfers + * @config: Offload configuration + * + * Peripheral drivers call this function to get an offload instance that meets + * the requirements specified in @config. If no suitable offload instance is + * available, -ENODEV is returned. + * + * Return: Offload instance or error on failure. + */ +struct spi_offload *devm_spi_offload_get(struct device *dev, + struct spi_device *spi, + const struct spi_offload_config *config) +{ + struct spi_controller_and_offload *resource; + int ret; + + if (!spi || !config) + return ERR_PTR(-EINVAL); + + if (!spi->controller->get_offload) + return ERR_PTR(-ENODEV); + + resource = kzalloc(sizeof(*resource), GFP_KERNEL); + if (!resource) + return ERR_PTR(-ENOMEM); + + resource->controller = spi->controller; + resource->offload = spi->controller->get_offload(spi, config); + if (IS_ERR(resource->offload)) { + kfree(resource); + return resource->offload; + } + + ret = devm_add_action_or_reset(dev, spi_offload_put, resource); + if (ret) + return ERR_PTR(ret); + + return resource->offload; +} +EXPORT_SYMBOL_GPL(devm_spi_offload_get); diff --git a/include/linux/spi/offload/consumer.h b/include/linux/spi/offload/consumer.h new file mode 100644 index 0000000000000000000000000000000000000000..05543dbedf3086fb4befcd149cff3c8c70a88825 --- /dev/null +++ b/include/linux/spi/offload/consumer.h @@ -0,0 +1,22 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (C) 2024 Analog Devices Inc. + * Copyright (C) 2024 BayLibre, SAS + */ + +#ifndef __LINUX_SPI_OFFLOAD_CONSUMER_H +#define __LINUX_SPI_OFFLOAD_CONSUMER_H + +#include <linux/module.h> +#include <linux/spi/offload/types.h> +#include <linux/types.h> + +MODULE_IMPORT_NS("SPI_OFFLOAD"); + +struct device; +struct spi_device; + +struct spi_offload *devm_spi_offload_get(struct device *dev, struct spi_device *spi, + const struct spi_offload_config *config); + +#endif /* __LINUX_SPI_OFFLOAD_CONSUMER_H */ diff --git a/include/linux/spi/offload/provider.h b/include/linux/spi/offload/provider.h new file mode 100644 index 0000000000000000000000000000000000000000..278c4edfcdb7b1f43870ca99d2ba252bf2820576 --- /dev/null +++ b/include/linux/spi/offload/provider.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (C) 2024 Analog Devices Inc. + * Copyright (C) 2024 BayLibre, SAS + */ + +#ifndef __LINUX_SPI_OFFLOAD_PROVIDER_H +#define __LINUX_SPI_OFFLOAD_PROVIDER_H + +#include <linux/module.h> +#include <linux/types.h> + +MODULE_IMPORT_NS("SPI_OFFLOAD"); + +struct device; + +struct spi_offload *devm_spi_offload_alloc(struct device *dev, size_t priv_size); + +#endif /* __LINUX_SPI_OFFLOAD_PROVIDER_H */ diff --git a/include/linux/spi/offload/types.h b/include/linux/spi/offload/types.h new file mode 100644 index 0000000000000000000000000000000000000000..a74f8d84541b10062353e81a638f05628b696394 --- /dev/null +++ b/include/linux/spi/offload/types.h @@ -0,0 +1,43 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (C) 2024 Analog Devices Inc. + * Copyright (C) 2024 BayLibre, SAS + */ + +#ifndef __LINUX_SPI_OFFLOAD_TYPES_H +#define __LINUX_SPI_OFFLOAD_TYPES_H + +#include <linux/types.h> + +struct device; + +/* Offload can be triggered by external hardware event. */ +#define SPI_OFFLOAD_CAP_TRIGGER BIT(0) +/* Offload can record and then play back TX data when triggered. */ +#define SPI_OFFLOAD_CAP_TX_STATIC_DATA BIT(1) +/* Offload can get TX data from an external stream source. */ +#define SPI_OFFLOAD_CAP_TX_STREAM_DMA BIT(2) +/* Offload can send RX data to an external stream sink. */ +#define SPI_OFFLOAD_CAP_RX_STREAM_DMA BIT(3) + +/** + * struct spi_offload_config - offload configuration + * + * This is used to request an offload with specific configuration. + */ +struct spi_offload_config { + /** @capability_flags: required capabilities. See %SPI_OFFLOAD_CAP_* */ + u32 capability_flags; +}; + +/** + * struct spi_offload - offload instance + */ +struct spi_offload { + /** @provider_dev: for get/put reference counting */ + struct device *provider_dev; + /** @priv: provider driver private data */ + void *priv; +}; + +#endif /* __LINUX_SPI_OFFLOAD_TYPES_H */ diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 8497f4747e24d4ecd85b74f49609ac1c82c73535..98bdc8c16c20521c0a94e5f72f5e71c4f6d7d11e 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -31,6 +31,8 @@ struct spi_transfer; struct spi_controller_mem_ops; struct spi_controller_mem_caps; struct spi_message; +struct spi_offload; +struct spi_offload_config; /* * INTERFACES between SPI master-side drivers and SPI slave protocol handlers, @@ -496,6 +498,10 @@ extern struct spi_device *spi_new_ancillary_device(struct spi_device *spi, u8 ch * @mem_ops: optimized/dedicated operations for interactions with SPI memory. * This field is optional and should only be implemented if the * controller has native support for memory like operations. + * @get_offload: callback for controllers with offload support to get matching + * offload instance. Implementations should return -ENODEV if no match is + * found. + * @put_offload: release the offload instance acquired by @get_offload. * @mem_caps: controller capabilities for the handling of memory operations. * @unprepare_message: undo any work done by prepare_message(). * @target_abort: abort the ongoing transfer request on an SPI target controller @@ -740,6 +746,10 @@ struct spi_controller { const struct spi_controller_mem_ops *mem_ops; const struct spi_controller_mem_caps *mem_caps; + struct spi_offload *(*get_offload)(struct spi_device *spi, + const struct spi_offload_config *config); + void (*put_offload)(struct spi_offload *offload); + /* GPIO chip select */ struct gpio_desc **cs_gpiods; bool use_gpio_descriptors; @@ -1108,6 +1118,7 @@ struct spi_transfer { * @state: for use by whichever driver currently owns the message * @opt_state: for use by whichever driver currently owns the message * @resources: for resource management when the SPI message is processed + * @offload: (optional) offload instance used by this message * * A @spi_message is used to execute an atomic sequence of data transfers, * each represented by a struct spi_transfer. The sequence is "atomic" @@ -1168,6 +1179,12 @@ struct spi_message { */ void *opt_state; + /* + * Optional offload instance used by this message. This must be set + * by the peripheral driver before calling spi_optimize_message(). + */ + struct spi_offload *offload; + /* List of spi_res resources when the SPI message is processed */ struct list_head resources; };