From: Alexander Usyskin <[email protected]>
Add GSC proxy driver. It to allows messaging between GSC component
on Intel on board graphics card and CSME device.
GSC and MEI
Signed-off-by: Alexander Usyskin <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
---
MAINTAINERS | 2 +-
drivers/misc/mei/Kconfig | 1 +
drivers/misc/mei/Makefile | 1 +
drivers/misc/mei/gsc_proxy/Kconfig | 13 ++
drivers/misc/mei/gsc_proxy/Makefile | 7 +
drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c | 205 +++++++++++++++++++++
6 files changed, 228 insertions(+), 1 deletion(-)
create mode 100644 drivers/misc/mei/gsc_proxy/Kconfig
create mode 100644 drivers/misc/mei/gsc_proxy/Makefile
create mode 100644 drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 5490b1f94803..01473c039412 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10561,7 +10561,7 @@ M: Tomas Winkler <[email protected]>
L: [email protected]
S: Supported
F: Documentation/driver-api/mei/*
-F: drivers/misc/mei/
+F: drivers/misc/mei/*
F: drivers/watchdog/mei_wdt.c
F: include/linux/mei_aux.h
F: include/linux/mei_cl_bus.h
diff --git a/drivers/misc/mei/Kconfig b/drivers/misc/mei/Kconfig
index d21486d69df2..92baed17a50b 100644
--- a/drivers/misc/mei/Kconfig
+++ b/drivers/misc/mei/Kconfig
@@ -62,4 +62,5 @@ config INTEL_MEI_GSC
source "drivers/misc/mei/hdcp/Kconfig"
source "drivers/misc/mei/pxp/Kconfig"
+source "drivers/misc/mei/gsc_proxy/Kconfig"
diff --git a/drivers/misc/mei/Makefile b/drivers/misc/mei/Makefile
index fb740d754900..14aee253ae48 100644
--- a/drivers/misc/mei/Makefile
+++ b/drivers/misc/mei/Makefile
@@ -30,3 +30,4 @@ CFLAGS_mei-trace.o = -I$(src)
obj-$(CONFIG_INTEL_MEI_HDCP) += hdcp/
obj-$(CONFIG_INTEL_MEI_PXP) += pxp/
+obj-$(CONFIG_INTEL_MEI_GSC_PROXY) += gsc_proxy/
diff --git a/drivers/misc/mei/gsc_proxy/Kconfig b/drivers/misc/mei/gsc_proxy/Kconfig
new file mode 100644
index 000000000000..9bc4486a6dad
--- /dev/null
+++ b/drivers/misc/mei/gsc_proxy/Kconfig
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022, Intel Corporation. All rights reserved.
+#
+config INTEL_MEI_GSC_PROXY
+ tristate "Intel GSC Proxy services of ME Interface"
+ select INTEL_MEI_ME
+ depends on DRM_I915
+ help
+ MEI Support for GSC Proxy Services on Intel platforms.
+
+ MEI GSC proxy enables messaging between GSC service on
+ Intel graphics on-board card and services on CSE (MEI)
+ firmware residing SoC or PCH.
diff --git a/drivers/misc/mei/gsc_proxy/Makefile b/drivers/misc/mei/gsc_proxy/Makefile
new file mode 100644
index 000000000000..fd7da77ad90a
--- /dev/null
+++ b/drivers/misc/mei/gsc_proxy/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (c) 2022, Intel Corporation. All rights reserved.
+#
+# Makefile - GSC Proxy client driver for Intel MEI Bus Driver.
+
+obj-$(CONFIG_INTEL_MEI_GSC_PROXY) += mei_gsc_proxy.o
diff --git a/drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c b/drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c
new file mode 100644
index 000000000000..4be6dc3ffdff
--- /dev/null
+++ b/drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c
@@ -0,0 +1,205 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2022 Intel Corporation
+ */
+
+/**
+ * DOC: MEI_GSC_PROXY Client Driver
+ *
+ * The mei_gsc_proxy driver acts as a translation layer between
+ * user (I915) and CSE FW services.
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/uuid.h>
+#include <linux/mei_cl_bus.h>
+#include <linux/component.h>
+#include <drm/drm_connector.h>
+#include <drm/i915_component.h>
+#include <drm/i915_gsc_proxy_mei_interface.h>
+
+/**
+ * mei_gsc_proxy_send - Sends a proxy message to ME FW.
+ * @dev: device corresponding to the mei_cl_device
+ * @buf: a message buffer to send
+ * @size: size of the message
+ * Return: bytes sent on Success, <0 on Failure
+ */
+static int mei_gsc_proxy_send(struct device *dev, const void *buf, size_t size)
+{
+ ssize_t ret;
+
+ if (!dev || !buf)
+ return -EINVAL;
+
+ ret = mei_cldev_send(to_mei_cl_device(dev), buf, size);
+ if (ret < 0)
+ dev_dbg(dev, "mei_cldev_send failed. %zd\n", ret);
+
+ return ret;
+}
+
+/**
+ * mei_gsc_proxy_recv - Receives a proxy message from ME FW.
+ * @dev: device corresponding to the mei_cl_device
+ * @buf: a message buffer to contain the received message
+ * @size: size of the buffer
+ * Return: bytes received on Success, <0 on Failure
+ */
+static int mei_gsc_proxy_recv(struct device *dev, void *buf, size_t size)
+{
+ ssize_t ret;
+
+ if (!dev || !buf)
+ return -EINVAL;
+
+ ret = mei_cldev_recv(to_mei_cl_device(dev), buf, size);
+ if (ret < 0)
+ dev_dbg(dev, "mei_cldev_recv failed. %zd\n", ret);
+
+ return ret;
+}
+
+static const struct i915_gsc_proxy_component_ops mei_gsc_proxy_ops = {
+ .owner = THIS_MODULE,
+ .send = mei_gsc_proxy_send,
+ .recv = mei_gsc_proxy_recv,
+};
+
+static int mei_component_master_bind(struct device *dev)
+{
+ struct mei_cl_device *cldev = to_mei_cl_device(dev);
+ struct i915_gsc_proxy_component *comp_master = mei_cldev_get_drvdata(cldev);
+
+ comp_master->ops = &mei_gsc_proxy_ops;
+ comp_master->mei_dev = dev;
+ return component_bind_all(dev, comp_master);
+}
+
+static void mei_component_master_unbind(struct device *dev)
+{
+ struct mei_cl_device *cldev = to_mei_cl_device(dev);
+ struct i915_gsc_proxy_component *comp_master = mei_cldev_get_drvdata(cldev);
+
+ component_unbind_all(dev, comp_master);
+}
+
+static const struct component_master_ops mei_component_master_ops = {
+ .bind = mei_component_master_bind,
+ .unbind = mei_component_master_unbind,
+};
+
+/**
+ * mei_gsc_proxy_component_match - compare function for matching mei.
+ *
+ * The function checks if the driver is i915, the subcomponent is SW Proxy
+ * and the grand parent of proxy and the parent of i915 are the same
+ * PCH device.
+ *
+ * @dev: master device
+ * @subcomponent: subcomponent to match (I915_COMPONENT_SWPROXY)
+ * @data: compare data (mei gsc proxy device)
+ *
+ * Return:
+ * * 1 - if components match
+ * * 0 - otherwise
+ */
+static int mei_gsc_proxy_component_match(struct device *dev, int subcomponent, void *data)
+{
+ struct device *base = data;
+
+ if (!dev || !dev->driver ||
+ strcmp(dev->driver->name, "i915") ||
+ subcomponent != I915_COMPONENT_GSC_PROXY)
+ return 0;
+
+ base = base->parent;
+ if (!base) /* mei device */
+ return 0;
+
+ base = base->parent; /* pci device */
+
+ dev = dev->parent;
+ return (base && dev && dev == base);
+}
+
+static int mei_gsc_proxy_probe(struct mei_cl_device *cldev, const struct mei_cl_device_id *id)
+{
+ struct i915_gsc_proxy_component *comp_master;
+ struct component_match *master_match = NULL;
+ int ret;
+
+ ret = mei_cldev_enable(cldev);
+ if (ret < 0) {
+ dev_err(&cldev->dev, "mei_cldev_enable Failed. %d\n", ret);
+ goto enable_err_exit;
+ }
+
+ comp_master = kzalloc(sizeof(*comp_master), GFP_KERNEL);
+ if (!comp_master) {
+ ret = -ENOMEM;
+ goto err_exit;
+ }
+
+ component_match_add_typed(&cldev->dev, &master_match,
+ mei_gsc_proxy_component_match, &cldev->dev);
+ if (IS_ERR_OR_NULL(master_match)) {
+ ret = -ENOMEM;
+ goto err_exit;
+ }
+
+ mei_cldev_set_drvdata(cldev, comp_master);
+ ret = component_master_add_with_match(&cldev->dev,
+ &mei_component_master_ops,
+ master_match);
+ if (ret < 0) {
+ dev_err(&cldev->dev, "Master comp add failed %d\n", ret);
+ goto err_exit;
+ }
+
+ return 0;
+
+err_exit:
+ mei_cldev_set_drvdata(cldev, NULL);
+ kfree(comp_master);
+ mei_cldev_disable(cldev);
+enable_err_exit:
+ return ret;
+}
+
+static void mei_gsc_proxy_remove(struct mei_cl_device *cldev)
+{
+ struct i915_gsc_proxy_component *comp_master = mei_cldev_get_drvdata(cldev);
+ int ret;
+
+ component_master_del(&cldev->dev, &mei_component_master_ops);
+ kfree(comp_master);
+ mei_cldev_set_drvdata(cldev, NULL);
+
+ ret = mei_cldev_disable(cldev);
+ if (ret)
+ dev_warn(&cldev->dev, "mei_cldev_disable() failed %d\n", ret);
+}
+
+#define MEI_GUID_GSC_PROXY GUID_INIT(0xf73db04, 0x97ab, 0x4125, \
+ 0xb8, 0x93, 0xe9, 0x4, 0xad, 0xd, 0x54, 0x64)
+
+static struct mei_cl_device_id mei_gsc_proxy_tbl[] = {
+ { .uuid = MEI_GUID_GSC_PROXY, .version = MEI_CL_VERSION_ANY },
+ { }
+};
+MODULE_DEVICE_TABLE(mei, mei_gsc_proxy_tbl);
+
+static struct mei_cl_driver mei_gsc_proxy_driver = {
+ .id_table = mei_gsc_proxy_tbl,
+ .name = KBUILD_MODNAME,
+ .probe = mei_gsc_proxy_probe,
+ .remove = mei_gsc_proxy_remove,
+};
+
+module_mei_cl_driver(mei_gsc_proxy_driver);
+
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("MEI GSC PROXY");
--
2.38.1
On Fri, Dec 23, 2022 at 12:02:14AM +0200, Tomas Winkler wrote:
> From: Alexander Usyskin <[email protected]>
>
> Add GSC proxy driver. It to allows messaging between GSC component
> on Intel on board graphics card and CSME device.
> GSC and MEI
>
> Signed-off-by: Alexander Usyskin <[email protected]>
> Signed-off-by: Tomas Winkler <[email protected]>
> ---
> MAINTAINERS | 2 +-
> drivers/misc/mei/Kconfig | 1 +
> drivers/misc/mei/Makefile | 1 +
> drivers/misc/mei/gsc_proxy/Kconfig | 13 ++
> drivers/misc/mei/gsc_proxy/Makefile | 7 +
> drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c | 205 +++++++++++++++++++++
Why a whole new subdirectory for a tiny 200 line file?
> +static int mei_gsc_proxy_component_match(struct device *dev, int subcomponent, void *data)
> +{
> + struct device *base = data;
> +
> + if (!dev || !dev->driver ||
> + strcmp(dev->driver->name, "i915") ||
I thought I had objected to this "let's poke around in a driver name for
a magic value" logic in the past. How do you know this is always going
to work?
> + subcomponent != I915_COMPONENT_GSC_PROXY)
> + return 0;
> +
> + base = base->parent;
> + if (!base) /* mei device */
> + return 0;
How can a device not have a parent?
> +
> + base = base->parent; /* pci device */
You don't know this is a pci device :(
If it is, then pass in a REAL pci device structure please.
> +
> + dev = dev->parent;
> + return (base && dev && dev == base);
I do not understand this statement at all, what are you doing here?
confused,
greg k-h
>
> Why a whole new subdirectory for a tiny 200 line file?
>
All drivers for devices on mei bus have private subdirectory.
This one just modelled on the existing examples.
If you say that this is not a good thing - can put it in the main mei directory.
> > +static int mei_gsc_proxy_component_match(struct device *dev, int
> subcomponent, void *data)
> > +{
> > + struct device *base = data;
> > +
> > + if (!dev || !dev->driver ||
> > + strcmp(dev->driver->name, "i915") ||
>
> I thought I had objected to this "let's poke around in a driver name for
> a magic value" logic in the past. How do you know this is always going
> to work?
All components that serve Intel graphics integrated into PCH should check
in their match that calling device is graphic card sitting on the same PCH.
The code below checks that i915 pci device and mei pci device (grandparent of our device on mei bus)
are children of the same parent, but there is no way to know if caller
is, indeed, graphic device. Easiest way is to check well-known device river name.
All i915 components doing this comparison.
This is a simplified scheme of relations between devices here:
/--- MEI PCI --- MEI --- GSC_PROXY
PCH ---
\--- GRAPHIC PCI --- I915
>
> > + subcomponent != I915_COMPONENT_GSC_PROXY)
> > + return 0;
> > +
> > + base = base->parent;
> > + if (!base) /* mei device */
> > + return 0;
>
> How can a device not have a parent?
This one should be proxy device on mei bus, so parent should be there always, can drop this check.
>
> > +
> > + base = base->parent; /* pci device */
>
> You don't know this is a pci device :(
This is more, like, note to explain on what level in above scheme we are now.
It should be mei pci device for match to succeed.
>
> If it is, then pass in a REAL pci device structure please.
>
> > +
> > + dev = dev->parent;
> > + return (base && dev && dev == base);
>
> I do not understand this statement at all, what are you doing here?
>
> confused,
Hope that scheme above explains the relations between devices.
>
> greg k-h
--
Thanks,
Sasha
On Wed, Dec 28, 2022 at 11:46:36AM +0000, Usyskin, Alexander wrote:
> >
> > Why a whole new subdirectory for a tiny 200 line file?
> >
> All drivers for devices on mei bus have private subdirectory.
> This one just modelled on the existing examples.
> If you say that this is not a good thing - can put it in the main mei directory.
Put it in the main mei directory, no need to split things up for no good
reason.
> > > +static int mei_gsc_proxy_component_match(struct device *dev, int
> > subcomponent, void *data)
> > > +{
> > > + struct device *base = data;
> > > +
> > > + if (!dev || !dev->driver ||
> > > + strcmp(dev->driver->name, "i915") ||
> >
> > I thought I had objected to this "let's poke around in a driver name for
> > a magic value" logic in the past. How do you know this is always going
> > to work?
>
> All components that serve Intel graphics integrated into PCH should check
> in their match that calling device is graphic card sitting on the same PCH.
And by looking at the driver name? That does not work, sorry. Get
access to the driver pointer please, that's the only way you know for
sure, right? And even then you shouldn't be messing with things in a
device you have no control over (i.e. a driver pointer or name.)
> The code below checks that i915 pci device and mei pci device (grandparent of our device on mei bus)
> are children of the same parent, but there is no way to know if caller
> is, indeed, graphic device. Easiest way is to check well-known device river name.
Again, that's a big abuse of the driver model, please do not do that.
You need to rely on the fact that you will NOT be called unless your
parent is of the correct type. That's all, no fancy layering violations
please.
> All i915 components doing this comparison.
> This is a simplified scheme of relations between devices here:
> /--- MEI PCI --- MEI --- GSC_PROXY
> PCH ---
> \--- GRAPHIC PCI --- I915
> >
> > > + subcomponent != I915_COMPONENT_GSC_PROXY)
> > > + return 0;
> > > +
> > > + base = base->parent;
> > > + if (!base) /* mei device */
> > > + return 0;
> >
> > How can a device not have a parent?
>
> This one should be proxy device on mei bus, so parent should be there always, can drop this check.
>
> >
> > > +
> > > + base = base->parent; /* pci device */
> >
> > You don't know this is a pci device :(
>
> This is more, like, note to explain on what level in above scheme we are now.
> It should be mei pci device for match to succeed.
Again, you can't know this and you should never poke around like this.
You don't control the parent of anything (hint, you just saved a
reference counted pointer without grabbing a reference count...)
Please don't abuse the driver model like this, it will cause long-term
problems for keeping this code alive and working properly.
thanks,
greg k-h
> -----Original Message-----
> From: Greg Kroah-Hartman <[email protected]>
> Sent: Thursday, January 19, 2023 18:21
> To: Usyskin, Alexander <[email protected]>
> Cc: Winkler, Tomas <[email protected]>; Lubart, Vitaly
> <[email protected]>; [email protected]
> Subject: Re: [char-misc-next] mei: gsc_proxy: add gsc proxy driver
>
> On Wed, Dec 28, 2022 at 11:46:36AM +0000, Usyskin, Alexander wrote:
> > >
> > > Why a whole new subdirectory for a tiny 200 line file?
> > >
> > All drivers for devices on mei bus have private subdirectory.
> > This one just modelled on the existing examples.
> > If you say that this is not a good thing - can put it in the main mei directory.
>
> Put it in the main mei directory, no need to split things up for no good
> reason.
>
All mei sub drivers are in sperate directories, this driver is indeed tiny, but I do prefer consistency,
In my view it is easier to maintain that way.
I believe all the bellow stuff you've already discussed in ths thread,.
https://lore.kernel.org/lkml/[email protected]/T/
I guess all that was explained there, so no need to repeat.
We'll try to see what can be done to make it more robust and your comments are more than valid,
but as the thread concludes probably the component framework needs to be addressed.
> > > > +static int mei_gsc_proxy_component_match(struct device *dev, int
> > > subcomponent, void *data)
> > > > +{
> > > > + struct device *base = data;
> > > > +
> > > > + if (!dev || !dev->driver ||
> > > > + strcmp(dev->driver->name, "i915") ||
> > >
> > > I thought I had objected to this "let's poke around in a driver name
> > > for a magic value" logic in the past. How do you know this is
> > > always going to work?
> >
> > All components that serve Intel graphics integrated into PCH should
> > check in their match that calling device is graphic card sitting on the same
> PCH.
>
> And by looking at the driver name? That does not work, sorry. Get access to
> the driver pointer please, that's the only way you know for sure, right? And
> even then you shouldn't be messing with things in a device you have no
> control over (i.e. a driver pointer or name.)
>
> > The code below checks that i915 pci device and mei pci device
> > (grandparent of our device on mei bus) are children of the same
> > parent, but there is no way to know if caller is, indeed, graphic device.
> Easiest way is to check well-known device river name.
>
> Again, that's a big abuse of the driver model, please do not do that.
>
> You need to rely on the fact that you will NOT be called unless your parent is
> of the correct type. That's all, no fancy layering violations please.
>
>
> > All i915 components doing this comparison.
> > This is a simplified scheme of relations between devices here:
> > /--- MEI PCI --- MEI --- GSC_PROXY PCH ---
> > \--- GRAPHIC PCI --- I915
> > >
> > > > + subcomponent != I915_COMPONENT_GSC_PROXY)
> > > > + return 0;
> > > > +
> > > > + base = base->parent;
> > > > + if (!base) /* mei device */
> > > > + return 0;
> > >
> > > How can a device not have a parent?
> >
> > This one should be proxy device on mei bus, so parent should be there
> always, can drop this check.
> >
> > >
> > > > +
> > > > + base = base->parent; /* pci device */
> > >
> > > You don't know this is a pci device :(
> >
> > This is more, like, note to explain on what level in above scheme we are
> now.
> > It should be mei pci device for match to succeed.
>
> Again, you can't know this and you should never poke around like this.
> You don't control the parent of anything (hint, you just saved a reference
> counted pointer without grabbing a reference count...)
>
> Please don't abuse the driver model like this, it will cause long-term problems
> for keeping this code alive and working properly.
>
> thanks,
>
> greg k-h
On Tue, Jan 24, 2023 at 09:05:19PM +0000, Winkler, Tomas wrote:
>
>
> > -----Original Message-----
> > From: Greg Kroah-Hartman <[email protected]>
> > Sent: Thursday, January 19, 2023 18:21
> > To: Usyskin, Alexander <[email protected]>
> > Cc: Winkler, Tomas <[email protected]>; Lubart, Vitaly
> > <[email protected]>; [email protected]
> > Subject: Re: [char-misc-next] mei: gsc_proxy: add gsc proxy driver
> >
> > On Wed, Dec 28, 2022 at 11:46:36AM +0000, Usyskin, Alexander wrote:
> > > >
> > > > Why a whole new subdirectory for a tiny 200 line file?
> > > >
> > > All drivers for devices on mei bus have private subdirectory.
> > > This one just modelled on the existing examples.
> > > If you say that this is not a good thing - can put it in the main mei directory.
> >
> > Put it in the main mei directory, no need to split things up for no good
> > reason.
> >
>
> All mei sub drivers are in sperate directories, this driver is indeed tiny, but I do prefer consistency,
> In my view it is easier to maintain that way.
Ok.
> I believe all the bellow stuff you've already discussed in ths thread,.
> https://lore.kernel.org/lkml/[email protected]/T/
> I guess all that was explained there, so no need to repeat.
>
> We'll try to see what can be done to make it more robust and your comments are more than valid,
> but as the thread concludes probably the component framework needs to be addressed.
Agreed, something needs to be done, you can't make these random
assumptions about the exact driver and device topology as that is not
how the driver model works at all (nor do you want it to work that way.)
thanks,
greg k-h