| Message ID | 20240916134928.3654054-2-alexander.usyskin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: add driver for Intel discrete graphics | expand |
On Mon, Sep 16, 2024 at 04:49:17PM +0300, Alexander Usyskin wrote: > Add auxiliary driver for intel discrete graphics > on-die spi device. This doesn't actually do anything AFAICT? It doesn't register with any subsystem or anything. Please don't submit empty boilerplate like this, submit a driver that is at least minimally useful - assuming some other patches in the series add functionality squash at least a basic set of functionality into this. This makes review and test easier. > +++ b/drivers/spi/spi-intel-dg.c > @@ -0,0 +1,142 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright(c) 2019-2024, Intel Corporation. All rights reserved. > + */ Please make the entire comment a C++ one so things look more intentional. > +struct intel_dg_spi { > + struct kref refcnt; > + void __iomem *base; > + size_t size; > + unsigned int nregions; > + struct { > + const char *name; > + u8 id; > + u64 offset; > + u64 size; > + } regions[]; Use __counted_by() for the benefit of the static checkers. > + size = sizeof(*spi) + sizeof(spi->regions[0]) * nregions; > + spi = kzalloc(size, GFP_KERNEL); Use at least array_size(). > + kref_init(&spi->refcnt); This refcount feels more complex than just freeing in the error and release paths, it's not a common pattern for drivers. > + spi->nregions = nregions; > + for (n = 0, i = 0; i < INTEL_DG_SPI_REGIONS; i++) { > + if (ispi->regions[i].name) { > + name_size = strlen(dev_name(&aux_dev->dev)) + > + strlen(ispi->regions[i].name) + 2; /* for point */ > + name = kzalloc(name_size, GFP_KERNEL); > + if (!name) > + continue; > + snprintf(name, name_size, "%s.%s", > + dev_name(&aux_dev->dev), ispi->regions[i].name); kasprintf(). > +static void intel_dg_spi_remove(struct auxiliary_device *aux_dev) > +{ > + struct intel_dg_spi *spi = dev_get_drvdata(&aux_dev->dev); > + > + if (!spi) > + return; > + > + dev_set_drvdata(&aux_dev->dev, NULL); If the above is doing anything there's a problem...
> > On Mon, Sep 16, 2024 at 04:49:17PM +0300, Alexander Usyskin wrote: > > > Add auxiliary driver for intel discrete graphics on-die spi device. > > This doesn't actually do anything AFAICT? It doesn't register with any > subsystem or anything. Please don't submit empty boilerplate like this, > submit a driver that is at least minimally useful - assuming some other > patches in the series add functionality squash at least a basic set of > functionality into this. This makes review and test easier. > > > +++ b/drivers/spi/spi-intel-dg.c > > @@ -0,0 +1,142 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright(c) 2019-2024, Intel Corporation. All rights reserved. > > + */ > > Please make the entire comment a C++ one so things look more intentional. This is how it is required by Linux spdx checker, > > > +struct intel_dg_spi { > > + struct kref refcnt; > > + void __iomem *base; > > + size_t size; > > + unsigned int nregions; > > + struct { > > + const char *name; > > + u8 id; > > + u64 offset; > > + u64 size; > > + } regions[]; > > Use __counted_by() for the benefit of the static checkers. Good point, it's a new C feature. > > > + size = sizeof(*spi) + sizeof(spi->regions[0]) * nregions; > > + spi = kzalloc(size, GFP_KERNEL); > > Use at least array_size(). Regions is not fixed size array, it will not work. > > > + kref_init(&spi->refcnt); > > This refcount feels more complex than just freeing in the error and release > paths, it's not a common pattern for drivers. What are you suggesting > > > + spi->nregions = nregions; > > + for (n = 0, i = 0; i < INTEL_DG_SPI_REGIONS; i++) { > > + if (ispi->regions[i].name) { > > + name_size = strlen(dev_name(&aux_dev->dev)) + > > + strlen(ispi->regions[i].name) + 2; /* for > point */ > > + name = kzalloc(name_size, GFP_KERNEL); > > + if (!name) > > + continue; > > + snprintf(name, name_size, "%s.%s", > > + dev_name(&aux_dev->dev), ispi- > >regions[i].name); > > kasprintf(). As I understand kasprintf allocates memory, this is not required here. > > > +static void intel_dg_spi_remove(struct auxiliary_device *aux_dev) { > > + struct intel_dg_spi *spi = dev_get_drvdata(&aux_dev->dev); > > + > > + if (!spi) > > + return; > > + > > + dev_set_drvdata(&aux_dev->dev, NULL); > > If the above is doing anything there's a problem... It makes sure the data is set to NULL.
On Thu, Sep 19, 2024 at 09:54:24AM +0000, Winkler, Tomas wrote: > > On Mon, Sep 16, 2024 at 04:49:17PM +0300, Alexander Usyskin wrote: > > > @@ -0,0 +1,142 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Copyright(c) 2019-2024, Intel Corporation. All rights reserved. > > > + */ > > Please make the entire comment a C++ one so things look more intentional. > This is how it is required by Linux spdx checker, There is no incompatibility between SPDX and what I'm asking for... > > > + size = sizeof(*spi) + sizeof(spi->regions[0]) * nregions; > > > + spi = kzalloc(size, GFP_KERNEL); > > Use at least array_size(). > Regions is not fixed size array, it will not work. Yes, that's the wrong helper - there is a relevent one though which I'm not remembering right now. > > > + kref_init(&spi->refcnt); > > This refcount feels more complex than just freeing in the error and release > > paths, it's not a common pattern for drivers. > What are you suggesting Just do normal open coded allocations, the reference counting is just obscure. > > > + if (ispi->regions[i].name) { > > > + name_size = strlen(dev_name(&aux_dev->dev)) + > > > + strlen(ispi->regions[i].name) + 2; /* for > > point */ > > > + name = kzalloc(name_size, GFP_KERNEL); > > > + if (!name) > > > + continue; > > > + snprintf(name, name_size, "%s.%s", > > > + dev_name(&aux_dev->dev), ispi- > > >regions[i].name); > > kasprintf(). > As I understand kasprintf allocates memory, this is not required here. There is a kzalloc() in the above code block? > > > +static void intel_dg_spi_remove(struct auxiliary_device *aux_dev) { > > > + struct intel_dg_spi *spi = dev_get_drvdata(&aux_dev->dev); > > > + if (!spi) > > > + return; > > > + dev_set_drvdata(&aux_dev->dev, NULL); > > If the above is doing anything there's a problem... o > It makes sure the data is set to NULL. Which is needed because...?
> > On Thu, Sep 19, 2024 at 09:54:24AM +0000, Winkler, Tomas wrote: > > > On Mon, Sep 16, 2024 at 04:49:17PM +0300, Alexander Usyskin wrote: > > > > > @@ -0,0 +1,142 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > +/* > > > > + * Copyright(c) 2019-2024, Intel Corporation. All rights reserved. > > > > + */ > > > > Please make the entire comment a C++ one so things look more > intentional. > > > This is how it is required by Linux spdx checker, > > There is no incompatibility between SPDX and what I'm asking for... > > > > > + size = sizeof(*spi) + sizeof(spi->regions[0]) * nregions; > > > > + spi = kzalloc(size, GFP_KERNEL); > > > > Use at least array_size(). > > > Regions is not fixed size array, it will not work. > > Yes, that's the wrong helper - there is a relevent one though which I'm not > remembering right now. I don't think there is one, you can allocate arrays but this is not the case here. > > > > > + kref_init(&spi->refcnt); > > > > This refcount feels more complex than just freeing in the error and > > > release paths, it's not a common pattern for drivers. > > > What are you suggesting > > Just do normal open coded allocations, the reference counting is just > obscure. The kref here is for reason so we don't need to hunt the close open, I frankly don't understand what is wrong with it, > > > > + if (ispi->regions[i].name) { > > > > + name_size = strlen(dev_name(&aux_dev->dev)) + > > > > + strlen(ispi->regions[i].name) + 2; /* for > > > point */ > > > > + name = kzalloc(name_size, GFP_KERNEL); > > > > + if (!name) > > > > + continue; > > > > + snprintf(name, name_size, "%s.%s", > > > > + dev_name(&aux_dev->dev), ispi- > > > >regions[i].name); > > > > kasprintf(). > > > As I understand kasprintf allocates memory, this is not required here. > > There is a kzalloc() in the above code block? Sorry you are right. > > > > > +static void intel_dg_spi_remove(struct auxiliary_device *aux_dev) { > > > > + struct intel_dg_spi *spi = dev_get_drvdata(&aux_dev->dev); > > > > > + if (!spi) > > > > + return; > > > > > + dev_set_drvdata(&aux_dev->dev, NULL); > > > > If the above is doing anything there's a problem... > o > > It makes sure the data is set to NULL. > > Which is needed because...? This is a boilerplate part, the content is consequent patches.
On 21/09/2024 14:00, Winkler, Tomas wrote: > > >> >> On Thu, Sep 19, 2024 at 09:54:24AM +0000, Winkler, Tomas wrote: >>>> On Mon, Sep 16, 2024 at 04:49:17PM +0300, Alexander Usyskin wrote: >> >>>>> @@ -0,0 +1,142 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0 >>>>> +/* >>>>> + * Copyright(c) 2019-2024, Intel Corporation. All rights reserved. >>>>> + */ >> >>>> Please make the entire comment a C++ one so things look more >> intentional. >> >>> This is how it is required by Linux spdx checker, >> >> There is no incompatibility between SPDX and what I'm asking for... >> >>>>> + size = sizeof(*spi) + sizeof(spi->regions[0]) * nregions; >>>>> + spi = kzalloc(size, GFP_KERNEL); >> >>>> Use at least array_size(). >> >>> Regions is not fixed size array, it will not work. >> >> Yes, that's the wrong helper - there is a relevent one though which I'm not >> remembering right now. > > > I don't think there is one, you can allocate arrays but this is not the case here. struct_size() probably. Regards, Tvrtko
On Sat, Sep 21, 2024 at 01:00:52PM +0000, Winkler, Tomas wrote: > > On Thu, Sep 19, 2024 at 09:54:24AM +0000, Winkler, Tomas wrote: > > > > On Mon, Sep 16, 2024 at 04:49:17PM +0300, Alexander Usyskin wrote: > > Just do normal open coded allocations, the reference counting is just > > obscure. > The kref here is for reason so we don't need to hunt the close open, I frankly don't understand > what is wrong with it, It's locking/refcounting stuff that looks nothing like any other SPI controller driver. Even if it works it's obviously fragile since the driver does surprising things which break assumptions that will be made by people not looking at this specific driver, and causes people to have to spend more effort figuring out what you're doing. If there is any benefit to doing this then open coding it in one specific driver is clearly not the right place to do it. > > > > > +static void intel_dg_spi_remove(struct auxiliary_device *aux_dev) { > > > > > + struct intel_dg_spi *spi = dev_get_drvdata(&aux_dev->dev); > > > > > + if (!spi) > > > > > + return; > > > > > + dev_set_drvdata(&aux_dev->dev, NULL); > > > > If the above is doing anything there's a problem... > > o > > > It makes sure the data is set to NULL. > > Which is needed because...? > This is a boilerplate part, the content is consequent patches. Which would come back to the issues created by the random splitting of the series were it not for the fact that if anything tries to look at the driver data of a removed device it's buggy, the reference is gone and the device may have been deallocated and it's certainly freed from the perspective of this user. Notice how other drivers don't do this. The driver core will also overwrite the driver data of released devices... At a high level a lot of the issues with this series is that both in terms of how it's been sent and what it's doing there's a bunch of things that look nothing like how we normally handle things. At best this means that problems are being solved at the wrong level, but it's hard to see that this is the case.
> On Sat, Sep 21, 2024 at 01:00:52PM +0000, Winkler, Tomas wrote: > > > On Thu, Sep 19, 2024 at 09:54:24AM +0000, Winkler, Tomas wrote: > > > > > On Mon, Sep 16, 2024 at 04:49:17PM +0300, Alexander Usyskin > wrote: > > > > Just do normal open coded allocations, the reference counting is just > > > obscure. > > > The kref here is for reason so we don't need to hunt the close open, I frankly > don't understand > > what is wrong with it, > > It's locking/refcounting stuff that looks nothing like any other SPI > controller driver. Even if it works it's obviously fragile since the > driver does surprising things which break assumptions that will be made > by people not looking at this specific driver, and causes people to have > to spend more effort figuring out what you're doing. If there is any > benefit to doing this then open coding it in one specific driver is > clearly not the right place to do it. > The reason for all this refcounting that the device itself is discrete card and the SPI memory backend can disappear at any moment leaving open connections dangling. Refcount ensures that object behind the MTD device will be freed only when the last user-space is disconnected. Is there any other model that can keep the object alive in this situation? - - Thanks, Sasha
On Wed, Sep 25, 2024 at 12:31:34PM +0000, Usyskin, Alexander wrote: > > It's locking/refcounting stuff that looks nothing like any other SPI > > controller driver. Even if it works it's obviously fragile since the > > driver does surprising things which break assumptions that will be made > > by people not looking at this specific driver, and causes people to have > > to spend more effort figuring out what you're doing. If there is any > > benefit to doing this then open coding it in one specific driver is > > clearly not the right place to do it. > The reason for all this refcounting that the device itself is discrete card and > the SPI memory backend can disappear at any moment leaving open connections dangling. > Refcount ensures that object behind the MTD device will be freed only > when the last user-space is disconnected. > Is there any other model that can keep the object alive in this situation? If devices connected to the SPI controller can be hotplugged then those devices should be registered and unregistered like any other spi_device would be, the driver core and relevant subsystems will deal with the lifetime issues. This is all invisible to the SPI controllers, the fact that things are being open coded in the driver should be a big warning sign.
diff --git a/MAINTAINERS b/MAINTAINERS index 333ed0718175..b2c1aa743bc6 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11214,6 +11214,13 @@ L: linux-kernel@vger.kernel.org S: Supported F: arch/x86/include/asm/intel-family.h +INTEL DISCRETE GRAPHIC SPI FLASH DRIVER +M: Alexander Usyskin <alexander.usyskin@intel.com> +L: linux-spi@vger.kernel.org +S: Supported +F: drivers/spi/spi-intel-dg.c +F: include/linux/intel_dg_spi_aux.h + INTEL DRM DISPLAY FOR XE AND I915 DRIVERS M: Jani Nikula <jani.nikula@linux.intel.com> M: Rodrigo Vivi <rodrigo.vivi@intel.com> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index ec1550c698d5..95d6667dc127 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -524,6 +524,17 @@ config SPI_INTEL_PLATFORM To compile this driver as a module, choose M here: the module will be called spi-intel-platform. +config SPI_INTEL_DG + tristate "Intel Discrete Graphic SPI flash driver" + depends on AUXILIARY_BUS + depends on MTD + help + This enables support for Intel Discrete Graphic SPI + auxiliary device. + + To compile this driver as a module, choose M here: the module + will be called spi-intel-dg. + config SPI_JCORE tristate "J-Core SPI Master" depends on OF && (SUPERH || COMPILE_TEST) diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index a9b1bc259b68..ae8398930f3b 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -71,6 +71,7 @@ obj-$(CONFIG_SPI_INGENIC) += spi-ingenic.o obj-$(CONFIG_SPI_INTEL) += spi-intel.o obj-$(CONFIG_SPI_INTEL_PCI) += spi-intel-pci.o obj-$(CONFIG_SPI_INTEL_PLATFORM) += spi-intel-platform.o +obj-$(CONFIG_SPI_INTEL_DG) += spi-intel-dg.o obj-$(CONFIG_SPI_LANTIQ_SSC) += spi-lantiq-ssc.o obj-$(CONFIG_SPI_JCORE) += spi-jcore.o obj-$(CONFIG_SPI_LJCA) += spi-ljca.o diff --git a/drivers/spi/spi-intel-dg.c b/drivers/spi/spi-intel-dg.c new file mode 100644 index 000000000000..4e302957f077 --- /dev/null +++ b/drivers/spi/spi-intel-dg.c @@ -0,0 +1,142 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright(c) 2019-2024, Intel Corporation. All rights reserved. + */ + +#include <linux/device.h> +#include <linux/intel_dg_spi_aux.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/string.h> +#include <linux/slab.h> +#include <linux/types.h> + +struct intel_dg_spi { + struct kref refcnt; + void __iomem *base; + size_t size; + unsigned int nregions; + struct { + const char *name; + u8 id; + u64 offset; + u64 size; + } regions[]; +}; + +static void intel_dg_spi_release(struct kref *kref) +{ + struct intel_dg_spi *spi = container_of(kref, struct intel_dg_spi, refcnt); + int i; + + pr_debug("freeing spi memory\n"); + for (i = 0; i < spi->nregions; i++) + kfree(spi->regions[i].name); + kfree(spi); +} + +static int intel_dg_spi_probe(struct auxiliary_device *aux_dev, + const struct auxiliary_device_id *aux_dev_id) +{ + struct intel_dg_spi_dev *ispi = auxiliary_dev_to_intel_dg_spi_dev(aux_dev); + struct device *device; + struct intel_dg_spi *spi; + unsigned int nregions; + unsigned int i, n; + size_t size; + char *name; + size_t name_size; + int ret; + + device = &aux_dev->dev; + + /* count available regions */ + for (nregions = 0, i = 0; i < INTEL_DG_SPI_REGIONS; i++) { + if (ispi->regions[i].name) + nregions++; + } + + if (!nregions) { + dev_err(device, "no regions defined\n"); + return -ENODEV; + } + + size = sizeof(*spi) + sizeof(spi->regions[0]) * nregions; + spi = kzalloc(size, GFP_KERNEL); + if (!spi) + return -ENOMEM; + + kref_init(&spi->refcnt); + + spi->nregions = nregions; + for (n = 0, i = 0; i < INTEL_DG_SPI_REGIONS; i++) { + if (ispi->regions[i].name) { + name_size = strlen(dev_name(&aux_dev->dev)) + + strlen(ispi->regions[i].name) + 2; /* for point */ + name = kzalloc(name_size, GFP_KERNEL); + if (!name) + continue; + snprintf(name, name_size, "%s.%s", + dev_name(&aux_dev->dev), ispi->regions[i].name); + spi->regions[n].name = name; + spi->regions[n].id = i; + n++; + } + } + + spi->base = devm_ioremap_resource(device, &ispi->bar); + if (IS_ERR(spi->base)) { + dev_err(device, "mmio not mapped\n"); + ret = PTR_ERR(spi->base); + goto err; + } + + dev_set_drvdata(&aux_dev->dev, spi); + + return 0; + +err: + kref_put(&spi->refcnt, intel_dg_spi_release); + return ret; +} + +static void intel_dg_spi_remove(struct auxiliary_device *aux_dev) +{ + struct intel_dg_spi *spi = dev_get_drvdata(&aux_dev->dev); + + if (!spi) + return; + + dev_set_drvdata(&aux_dev->dev, NULL); + + kref_put(&spi->refcnt, intel_dg_spi_release); +} + +static const struct auxiliary_device_id intel_dg_spi_id_table[] = { + { + .name = "i915.spi", + }, + { + .name = "xe.spi", + }, + { + /* sentinel */ + } +}; +MODULE_DEVICE_TABLE(auxiliary, intel_dg_spi_id_table); + +static struct auxiliary_driver intel_dg_spi_driver = { + .probe = intel_dg_spi_probe, + .remove = intel_dg_spi_remove, + .driver = { + /* auxiliary_driver_register() sets .name to be the modname */ + }, + .id_table = intel_dg_spi_id_table +}; + +module_auxiliary_driver(intel_dg_spi_driver); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Intel Corporation"); +MODULE_DESCRIPTION("Intel DGFX SPI driver"); diff --git a/include/linux/intel_dg_spi_aux.h b/include/linux/intel_dg_spi_aux.h new file mode 100644 index 000000000000..d4c3830d56d6 --- /dev/null +++ b/include/linux/intel_dg_spi_aux.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Copyright(c) 2019-2024, Intel Corporation. All rights reserved. + */ + +#ifndef __INTEL_DG_SPI_AUX_H__ +#define __INTEL_DG_SPI_AUX_H__ + +#include <linux/auxiliary_bus.h> + +#define INTEL_DG_SPI_REGIONS 13 + +struct intel_dg_spi_region { + const char *name; +}; + +struct intel_dg_spi_dev { + struct auxiliary_device aux_dev; + bool writeable_override; + struct resource bar; + const struct intel_dg_spi_region *regions; +}; + +#define auxiliary_dev_to_intel_dg_spi_dev(auxiliary_dev) \ + container_of(auxiliary_dev, struct intel_dg_spi_dev, aux_dev) + +#endif /* __INTEL_DG_SPI_AUX_H__ */