| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aF1XTr2y1EmkRT_8@smile.fi.intel.com>
Date: Thu, 26 Jun 2025 17:21:02 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@...el.com>,
Thomas Hellström <thomas.hellstrom@...ux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@...el.com>,
Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
Mika Westerberg <mika.westerberg@...ux.intel.com>,
Jan Dabros <jsd@...ihalf.com>, Andi Shyti <andi.shyti@...nel.org>,
Raag Jadav <raag.jadav@...el.com>,
"Tauro, Riana" <riana.tauro@...el.com>,
"Adatrao, Srinivasa" <srinivasa.adatrao@...el.com>,
"Michael J. Ruhl" <michael.j.ruhl@...el.com>,
intel-xe@...ts.freedesktop.org, linux-i2c@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/4] drm/xe: Support for I2C attached MCUs
On Thu, Jun 26, 2025 at 04:56:07PM +0300, Heikki Krogerus wrote:
> Adding adaption/glue layer where the I2C host adapter
> (Synopsys DesignWare I2C adapter) and the I2C clients (the
> microcontroller units) are enumerated.
>
> The microcontroller units (MCU) that are attached to the GPU
> depend on the OEM. The initially supported MCU will be the
> Add-In Management Controller (AMC).
...
> +static int xe_i2c_register_adapter(struct xe_i2c *i2c)
> +{
> + struct pci_dev *pci = to_pci_dev(i2c->drm_dev);
> + struct platform_device *pdev;
> + struct fwnode_handle *fwnode;
> + int ret;
> +
> + fwnode = fwnode_create_software_node(xe_i2c_adapter_properties, NULL);
> + if (!fwnode)
> + return -ENOMEM;
> +
> + /*
> + * Not using platform_device_register_full() here because we don't have
> + * a handle to the platform_device before it returns. xe_i2c_notifier()
> + * uses that handle, but it may be called before
> + * platform_device_register_full() is done.
> + */
> + pdev = platform_device_alloc(adapter_name, pci_dev_id(pci));
> + if (!pdev) {
> + ret = -ENOMEM;
> + goto err_fwnode_remove;
> + }
> +
> + if (i2c->adapter_irq) {
> + struct resource res = { };
> +
> + res.start = i2c->adapter_irq;
> + res.name = "xe_i2c";
> + res.flags = IORESOURCE_IRQ;
struct resource res;
res = DEFINE_RES_IRQ_NAMED(i2c->adapter_irq, "xe_i2c");
> + ret = platform_device_add_resources(pdev, &res, 1);
> + if (ret)
> + goto err_pdev_put;
> + }
> +
> + pdev->dev.parent = i2c->drm_dev;
> + pdev->dev.fwnode = fwnode;
> + i2c->adapter_node = fwnode;
> + i2c->pdev = pdev;
> +
> + ret = platform_device_add(pdev);
> + if (ret)
> + goto err_pdev_put;
> +
> + return 0;
> +
> +err_pdev_put:
> + platform_device_put(pdev);
> +err_fwnode_remove:
> + fwnode_remove_software_node(fwnode);
> +
> + return ret;
> +}
...
> +static int xe_i2c_irq_map(struct irq_domain *h, unsigned int virq,
> + irq_hw_number_t hw_irq_num)
> +{
> + irq_set_chip_and_handler(virq, &dummy_irq_chip, handle_simple_irq);
Wondering if you need to setup a custom lockdep class here.
> + return 0;
> +}
...
> +static void xe_i2c_remove_irq(struct xe_i2c *i2c)
> +{
> + if (i2c->irqdomain) {
if (!i2c->irqdomain)
return;
> + irq_dispose_mapping(i2c->adapter_irq);
> + irq_domain_remove(i2c->irqdomain);
> + }
> +}
...
> +static void xe_i2c_remove(void *data)
> +{
> + struct xe_i2c *i2c = data;
> + int i;
unsigned?
> + for (i = 0; i < XE_I2C_MAX_CLIENTS; i++)
> + i2c_unregister_device(i2c->client[i]);
> +
> + bus_unregister_notifier(&i2c_bus_type, &i2c->bus_notifier);
> + xe_i2c_unregister_adapter(i2c);
> + xe_i2c_remove_irq(i2c);
> +}
...
> +int xe_i2c_probe(struct xe_device *xe)
> +{
> + struct xe_i2c_endpoint ep;
> + struct regmap *regmap;
> + struct xe_i2c *i2c;
> + int ret;
> +
> + xe_i2c_read_endpoint(xe_root_tile_mmio(xe), &ep);
> + if (ep.cookie != XE_I2C_EP_COOKIE_DEVICE)
> + return 0;
> +
> + i2c = devm_kzalloc(xe->drm.dev, sizeof(*i2c), GFP_KERNEL);
> + if (!i2c)
> + return -ENOMEM;
> +
> + INIT_WORK(&i2c->work, xe_i2c_client_work);
> + i2c->mmio = xe_root_tile_mmio(xe);
> + i2c->drm_dev = xe->drm.dev;
> + i2c->ep = ep;
> + regmap = devm_regmap_init(i2c->drm_dev, NULL, i2c, &i2c_regmap_config);
Use of i2c->drm_dev makes harder to maintain and understand the code.
Managed resources should be carefully attached to the correct device,
otherwise it's inevitable object lifetime related issues.
With
struct device *dev = xe->drm.dev;
and using local dev, it becomes easier to get and avoid such subtle mistakes.
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + i2c->bus_notifier.notifier_call = xe_i2c_notifier;
> + ret = bus_register_notifier(&i2c_bus_type, &i2c->bus_notifier);
> + if (ret)
> + return ret;
> +
> + ret = xe_i2c_create_irq(i2c);
> + if (ret)
> + goto err_unregister_notifier;
> +
> + ret = xe_i2c_register_adapter(i2c);
> + if (ret)
> + goto err_remove_irq;
> +
> + return devm_add_action_or_reset(i2c->drm_dev, xe_i2c_remove, i2c);
> +
> +err_remove_irq:
> + xe_i2c_remove_irq(i2c);
> +
> +err_unregister_notifier:
> + bus_unregister_notifier(&i2c_bus_type, &i2c->bus_notifier);
> +
> + return ret;
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists