| Message ID | 20250424132536.3043825-3-alexander.usyskin@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | mtd: add driver for Intel discrete graphics | expand |
On Thu, Apr 24, 2025 at 04:25:26PM +0300, Alexander Usyskin wrote: > Add auxiliary driver for intel discrete graphics > non-volatile memory device. ... > +static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev, > + const struct auxiliary_device_id *aux_dev_id) > +{ > + struct intel_dg_nvm_dev *invm = auxiliary_dev_to_intel_dg_nvm_dev(aux_dev); > + struct device *device; > + struct intel_dg_nvm *nvm; > + unsigned int nregions; > + unsigned int i, n; > + char *name; Perhaps move this to the loop it is being used in? > + int ret; > + > + device = &aux_dev->dev; > + > + /* count available regions */ > + for (nregions = 0, i = 0; i < INTEL_DG_NVM_REGIONS; i++) { > + if (invm->regions[i].name) > + nregions++; > + } > + > + if (!nregions) { > + dev_err(device, "no regions defined\n"); > + return -ENODEV; > + } > + > + nvm = kzalloc(struct_size(nvm, regions, nregions), GFP_KERNEL); > + if (!nvm) > + return -ENOMEM; > + > + kref_init(&nvm->refcnt); > + > + nvm->nregions = nregions; Is this assignment useful? > + for (n = 0, i = 0; i < INTEL_DG_NVM_REGIONS; i++) { > + if (!invm->regions[i].name) > + continue; > + > + name = kasprintf(GFP_KERNEL, "%s.%s", > + dev_name(&aux_dev->dev), invm->regions[i].name); > + if (!name) > + continue; > + nvm->regions[n].name = name; > + nvm->regions[n].id = i; > + n++; > + } > + nvm->nregions = n; /* in case where kasprintf fail */ Considering kasprintf failure, should we move forward if n == 0? > + nvm->base = devm_ioremap_resource(device, &invm->bar); > + if (IS_ERR(nvm->base)) { > + dev_err(device, "mmio not mapped\n"); Is this useful? Perhaps the helper already does it for us. > + ret = PTR_ERR(nvm->base); > + goto err; > + } > + > + dev_set_drvdata(&aux_dev->dev, nvm); > + > + return 0; > + > +err: > + kref_put(&nvm->refcnt, intel_dg_nvm_release); > + return ret; > +} > + > +static void intel_dg_mtd_remove(struct auxiliary_device *aux_dev) > +{ > + struct intel_dg_nvm *nvm = dev_get_drvdata(&aux_dev->dev); > + > + if (!nvm) > + return; Are we expecting this? > + dev_set_drvdata(&aux_dev->dev, NULL); Do we need this? > + kref_put(&nvm->refcnt, intel_dg_nvm_release); > +} > + > +static const struct auxiliary_device_id intel_dg_mtd_id_table[] = { > + { > + .name = "i915.nvm", > + }, > + { > + .name = "xe.nvm", > + }, > + { > + /* sentinel */ > + } > +}; > +MODULE_DEVICE_TABLE(auxiliary, intel_dg_mtd_id_table); > + > +static struct auxiliary_driver intel_dg_mtd_driver = { > + .probe = intel_dg_mtd_probe, > + .remove = intel_dg_mtd_remove, > + .driver = { > + /* auxiliary_driver_register() sets .name to be the modname */ > + }, > + .id_table = intel_dg_mtd_id_table > +}; > + Nit: Redundant blank line. > +module_auxiliary_driver(intel_dg_mtd_driver); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Intel Corporation"); > +MODULE_DESCRIPTION("Intel DGFX MTD driver"); > diff --git a/include/linux/intel_dg_nvm_aux.h b/include/linux/intel_dg_nvm_aux.h > new file mode 100644 > index 000000000000..68df634c994c > --- /dev/null > +++ b/include/linux/intel_dg_nvm_aux.h > @@ -0,0 +1,27 @@ > +/* SPDX-License-Identifier: MIT */ > +/* > + * Copyright(c) 2019-2025, Intel Corporation. All rights reserved. > + */ > + > +#ifndef __INTEL_DG_NVM_AUX_H__ > +#define __INTEL_DG_NVM_AUX_H__ > + > +#include <linux/auxiliary_bus.h> Missing types.h, container_of.h > +#define INTEL_DG_NVM_REGIONS 13 > + > +struct intel_dg_nvm_region { > + const char *name; > +}; > + > +struct intel_dg_nvm_dev { > + struct auxiliary_device aux_dev; > + bool writable_override; > + struct resource bar; > + const struct intel_dg_nvm_region *regions; > +}; > + > +#define auxiliary_dev_to_intel_dg_nvm_dev(auxiliary_dev) \ > + container_of(auxiliary_dev, struct intel_dg_nvm_dev, aux_dev) > + > +#endif /* __INTEL_DG_NVM_AUX_H__ */ > -- > 2.43.0 >
> Subject: Re: [PATCH v9 02/12] mtd: add driver for intel graphics non-volatile > memory device > > On Thu, Apr 24, 2025 at 04:25:26PM +0300, Alexander Usyskin wrote: > > Add auxiliary driver for intel discrete graphics > > non-volatile memory device. > > ... > > > +static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev, > > + const struct auxiliary_device_id *aux_dev_id) > > +{ > > + struct intel_dg_nvm_dev *invm = > auxiliary_dev_to_intel_dg_nvm_dev(aux_dev); > > + struct device *device; > > + struct intel_dg_nvm *nvm; > > + unsigned int nregions; > > + unsigned int i, n; > > + char *name; > > Perhaps move this to the loop it is being used in? > Will do > > + int ret; > > + > > + device = &aux_dev->dev; > > + > > + /* count available regions */ > > + for (nregions = 0, i = 0; i < INTEL_DG_NVM_REGIONS; i++) { > > + if (invm->regions[i].name) > > + nregions++; > > + } > > + > > + if (!nregions) { > > + dev_err(device, "no regions defined\n"); > > + return -ENODEV; > > + } > > + > > + nvm = kzalloc(struct_size(nvm, regions, nregions), GFP_KERNEL); > > + if (!nvm) > > + return -ENOMEM; > > + > > + kref_init(&nvm->refcnt); > > + > > + nvm->nregions = nregions; > > Is this assignment useful? Nope, removing > > > + for (n = 0, i = 0; i < INTEL_DG_NVM_REGIONS; i++) { > > + if (!invm->regions[i].name) > > + continue; > > + > > + name = kasprintf(GFP_KERNEL, "%s.%s", > > + dev_name(&aux_dev->dev), invm- > >regions[i].name); > > + if (!name) > > + continue; > > + nvm->regions[n].name = name; > > + nvm->regions[n].id = i; > > + n++; > > + } > > + nvm->nregions = n; /* in case where kasprintf fail */ > > Considering kasprintf failure, should we move forward if n == 0? Not sure if adding exit path here adds something positive to driver other than complexity. > > > + nvm->base = devm_ioremap_resource(device, &invm->bar); > > + if (IS_ERR(nvm->base)) { > > + dev_err(device, "mmio not mapped\n"); > > Is this useful? Perhaps the helper already does it for us. Yes, will remove > > > + ret = PTR_ERR(nvm->base); > > + goto err; > > + } > > + > > + dev_set_drvdata(&aux_dev->dev, nvm); > > + > > + return 0; > > + > > +err: > > + kref_put(&nvm->refcnt, intel_dg_nvm_release); > > + return ret; > > +} > > + > > +static void intel_dg_mtd_remove(struct auxiliary_device *aux_dev) > > +{ > > + struct intel_dg_nvm *nvm = dev_get_drvdata(&aux_dev->dev); > > + > > + if (!nvm) > > + return; > > Are we expecting this? > > > + dev_set_drvdata(&aux_dev->dev, NULL); > > Do we need this? Is there guaranty by auxiliary device that after release nothing is called? > > > + kref_put(&nvm->refcnt, intel_dg_nvm_release); > > +} > > + > > +static const struct auxiliary_device_id intel_dg_mtd_id_table[] = { > > + { > > + .name = "i915.nvm", > > + }, > > + { > > + .name = "xe.nvm", > > + }, > > + { > > + /* sentinel */ > > + } > > +}; > > +MODULE_DEVICE_TABLE(auxiliary, intel_dg_mtd_id_table); > > + > > +static struct auxiliary_driver intel_dg_mtd_driver = { > > + .probe = intel_dg_mtd_probe, > > + .remove = intel_dg_mtd_remove, > > + .driver = { > > + /* auxiliary_driver_register() sets .name to be the modname > */ > > + }, > > + .id_table = intel_dg_mtd_id_table > > +}; > > > + > > Nit: Redundant blank line. Will remove > > > +module_auxiliary_driver(intel_dg_mtd_driver); > > + > > +MODULE_LICENSE("GPL"); > > +MODULE_AUTHOR("Intel Corporation"); > > +MODULE_DESCRIPTION("Intel DGFX MTD driver"); > > diff --git a/include/linux/intel_dg_nvm_aux.h > b/include/linux/intel_dg_nvm_aux.h > > new file mode 100644 > > index 000000000000..68df634c994c > > --- /dev/null > > +++ b/include/linux/intel_dg_nvm_aux.h > > @@ -0,0 +1,27 @@ > > +/* SPDX-License-Identifier: MIT */ > > +/* > > + * Copyright(c) 2019-2025, Intel Corporation. All rights reserved. > > + */ > > + > > +#ifndef __INTEL_DG_NVM_AUX_H__ > > +#define __INTEL_DG_NVM_AUX_H__ > > + > > +#include <linux/auxiliary_bus.h> > > Missing types.h, container_of.h Will add > > > +#define INTEL_DG_NVM_REGIONS 13 > > + > > +struct intel_dg_nvm_region { > > + const char *name; > > +}; > > + > > +struct intel_dg_nvm_dev { > > + struct auxiliary_device aux_dev; > > + bool writable_override; > > + struct resource bar; > > + const struct intel_dg_nvm_region *regions; > > +}; > > + > > +#define auxiliary_dev_to_intel_dg_nvm_dev(auxiliary_dev) \ > > + container_of(auxiliary_dev, struct intel_dg_nvm_dev, aux_dev) > > + > > +#endif /* __INTEL_DG_NVM_AUX_H__ */ > > -- > > 2.43.0 > >
On Thu, May 15, 2025 at 03:41:08PM +0530, Usyskin, Alexander wrote: > > On Thu, Apr 24, 2025 at 04:25:26PM +0300, Alexander Usyskin wrote: > > > Add auxiliary driver for intel discrete graphics > > > non-volatile memory device. ... > > > + for (n = 0, i = 0; i < INTEL_DG_NVM_REGIONS; i++) { > > > + if (!invm->regions[i].name) > > > + continue; > > > + > > > + name = kasprintf(GFP_KERNEL, "%s.%s", > > > + dev_name(&aux_dev->dev), invm- > > >regions[i].name); > > > + if (!name) > > > + continue; > > > + nvm->regions[n].name = name; > > > + nvm->regions[n].id = i; > > > + n++; > > > + } > > > + nvm->nregions = n; /* in case where kasprintf fail */ > > > > Considering kasprintf failure, should we move forward if n == 0? > Not sure if adding exit path here adds something positive to driver > other than complexity. With an error path already in place it shouldn't be too complex, but upto you. ... > > > +static void intel_dg_mtd_remove(struct auxiliary_device *aux_dev) > > > +{ > > > + struct intel_dg_nvm *nvm = dev_get_drvdata(&aux_dev->dev); > > > + > > > + if (!nvm) > > > + return; > > > > Are we expecting this? > > > > > + dev_set_drvdata(&aux_dev->dev, NULL); > > > > Do we need this? > Is there guaranty by auxiliary device that after release nothing is called? Any reports/link to read about such issues? My understanding is that driver ->remove() callbacks are bus lock held and there won't be an active instance to be called after unbind. Raag
diff --git a/MAINTAINERS b/MAINTAINERS index dc7ea662bcea..7548561c78c0 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11842,6 +11842,13 @@ L: linux-kernel@vger.kernel.org S: Supported F: arch/x86/include/asm/intel-family.h +INTEL DISCRETE GRAPHICS NVM MTD DRIVER +M: Alexander Usyskin <alexander.usyskin@intel.com> +L: linux-mtd@lists.infradead.org +S: Supported +F: drivers/mtd/devices/mtd_intel_dg.c +F: include/linux/intel_dg_nvm_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/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig index ff2f9e55ef28..59be6d3f0d32 100644 --- a/drivers/mtd/devices/Kconfig +++ b/drivers/mtd/devices/Kconfig @@ -183,6 +183,17 @@ config MTD_POWERNV_FLASH platforms from Linux. This device abstracts away the firmware interface for flash access. +config MTD_INTEL_DG + tristate "Intel Discrete Graphics non-volatile memory driver" + depends on AUXILIARY_BUS + depends on MTD + help + This provides an MTD device to access Intel Discrete Graphics + non-volatile memory. + + To compile this driver as a module, choose M here: the module + will be called mtd-intel-dg. + comment "Disk-On-Chip Device Drivers" config MTD_DOCG3 diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile index d11eb2b8b6f8..9fe4ce9cffde 100644 --- a/drivers/mtd/devices/Makefile +++ b/drivers/mtd/devices/Makefile @@ -18,6 +18,7 @@ obj-$(CONFIG_MTD_SST25L) += sst25l.o obj-$(CONFIG_MTD_BCM47XXSFLASH) += bcm47xxsflash.o obj-$(CONFIG_MTD_ST_SPI_FSM) += st_spi_fsm.o obj-$(CONFIG_MTD_POWERNV_FLASH) += powernv_flash.o +obj-$(CONFIG_MTD_INTEL_DG) += mtd_intel_dg.o CFLAGS_docg3.o += -I$(src) diff --git a/drivers/mtd/devices/mtd_intel_dg.c b/drivers/mtd/devices/mtd_intel_dg.c new file mode 100644 index 000000000000..963a88cacc6c --- /dev/null +++ b/drivers/mtd/devices/mtd_intel_dg.c @@ -0,0 +1,138 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright(c) 2019-2025, Intel Corporation. All rights reserved. + */ + +#include <linux/device.h> +#include <linux/intel_dg_nvm_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_nvm { + struct kref refcnt; + void __iomem *base; + size_t size; + unsigned int nregions; + struct { + const char *name; + u8 id; + u64 offset; + u64 size; + } regions[] __counted_by(nregions); +}; + +static void intel_dg_nvm_release(struct kref *kref) +{ + struct intel_dg_nvm *nvm = container_of(kref, struct intel_dg_nvm, refcnt); + int i; + + pr_debug("freeing intel_dg nvm\n"); + for (i = 0; i < nvm->nregions; i++) + kfree(nvm->regions[i].name); + kfree(nvm); +} + +static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev, + const struct auxiliary_device_id *aux_dev_id) +{ + struct intel_dg_nvm_dev *invm = auxiliary_dev_to_intel_dg_nvm_dev(aux_dev); + struct device *device; + struct intel_dg_nvm *nvm; + unsigned int nregions; + unsigned int i, n; + char *name; + int ret; + + device = &aux_dev->dev; + + /* count available regions */ + for (nregions = 0, i = 0; i < INTEL_DG_NVM_REGIONS; i++) { + if (invm->regions[i].name) + nregions++; + } + + if (!nregions) { + dev_err(device, "no regions defined\n"); + return -ENODEV; + } + + nvm = kzalloc(struct_size(nvm, regions, nregions), GFP_KERNEL); + if (!nvm) + return -ENOMEM; + + kref_init(&nvm->refcnt); + + nvm->nregions = nregions; + for (n = 0, i = 0; i < INTEL_DG_NVM_REGIONS; i++) { + if (!invm->regions[i].name) + continue; + + name = kasprintf(GFP_KERNEL, "%s.%s", + dev_name(&aux_dev->dev), invm->regions[i].name); + if (!name) + continue; + nvm->regions[n].name = name; + nvm->regions[n].id = i; + n++; + } + nvm->nregions = n; /* in case where kasprintf fail */ + + nvm->base = devm_ioremap_resource(device, &invm->bar); + if (IS_ERR(nvm->base)) { + dev_err(device, "mmio not mapped\n"); + ret = PTR_ERR(nvm->base); + goto err; + } + + dev_set_drvdata(&aux_dev->dev, nvm); + + return 0; + +err: + kref_put(&nvm->refcnt, intel_dg_nvm_release); + return ret; +} + +static void intel_dg_mtd_remove(struct auxiliary_device *aux_dev) +{ + struct intel_dg_nvm *nvm = dev_get_drvdata(&aux_dev->dev); + + if (!nvm) + return; + + dev_set_drvdata(&aux_dev->dev, NULL); + + kref_put(&nvm->refcnt, intel_dg_nvm_release); +} + +static const struct auxiliary_device_id intel_dg_mtd_id_table[] = { + { + .name = "i915.nvm", + }, + { + .name = "xe.nvm", + }, + { + /* sentinel */ + } +}; +MODULE_DEVICE_TABLE(auxiliary, intel_dg_mtd_id_table); + +static struct auxiliary_driver intel_dg_mtd_driver = { + .probe = intel_dg_mtd_probe, + .remove = intel_dg_mtd_remove, + .driver = { + /* auxiliary_driver_register() sets .name to be the modname */ + }, + .id_table = intel_dg_mtd_id_table +}; + +module_auxiliary_driver(intel_dg_mtd_driver); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Intel Corporation"); +MODULE_DESCRIPTION("Intel DGFX MTD driver"); diff --git a/include/linux/intel_dg_nvm_aux.h b/include/linux/intel_dg_nvm_aux.h new file mode 100644 index 000000000000..68df634c994c --- /dev/null +++ b/include/linux/intel_dg_nvm_aux.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Copyright(c) 2019-2025, Intel Corporation. All rights reserved. + */ + +#ifndef __INTEL_DG_NVM_AUX_H__ +#define __INTEL_DG_NVM_AUX_H__ + +#include <linux/auxiliary_bus.h> + +#define INTEL_DG_NVM_REGIONS 13 + +struct intel_dg_nvm_region { + const char *name; +}; + +struct intel_dg_nvm_dev { + struct auxiliary_device aux_dev; + bool writable_override; + struct resource bar; + const struct intel_dg_nvm_region *regions; +}; + +#define auxiliary_dev_to_intel_dg_nvm_dev(auxiliary_dev) \ + container_of(auxiliary_dev, struct intel_dg_nvm_dev, aux_dev) + +#endif /* __INTEL_DG_NVM_AUX_H__ */