diff mbox series

[v4,05/15] mei: pxp: add command streamer API to the PXP driver

Message ID 20220909001612.728451-6-daniele.ceraolospurio@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: HuC loading for DG2 | expand

Commit Message

Daniele Ceraolo Spurio Sept. 9, 2022, 12:16 a.m. UTC 
From: Vitaly Lubart <vitaly.lubart@intel.com>

The discrete graphics card with GSC firmware
using command streamer API hence it requires to enhance
pxp module with the new gsc_command() handler.

The handler is implemented via mei_pxp_gsc_command() which is
just a thin wrapper around mei_cldev_send_gsc_command()

V2:
 1. More detailed commit message
 2. Fix typo in the comments
V3: Rebase
V4:
1. Use forward declaration for struct scatterlist (Jani)
2. Drop double 'just' in the commit message

Signed-off-by: Vitaly Lubart <vitaly.lubart@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/misc/mei/pxp/mei_pxp.c       | 28 ++++++++++++++++++++++++++++
 include/drm/i915_pxp_tee_interface.h |  5 +++++
 2 files changed, 33 insertions(+)

Comments

Greg KH Sept. 9, 2022, 6:14 a.m. UTC | #1 
On Thu, Sep 08, 2022 at 05:16:02PM -0700, Daniele Ceraolo Spurio wrote:
> +static ssize_t mei_pxp_gsc_command(struct device *dev, u8 client_id, u32 fence_id,
> +				   struct scatterlist *sg_in, size_t total_in_len,
> +				   struct scatterlist *sg_out)
> +{
> +	struct mei_cl_device *cldev;
> +
> +	if (!dev || !sg_in || !sg_out)
> +		return -EINVAL;

How can these ever be NULL?  Doesn't the core control this, so why would
that happen?

Don't check for things that can never happen.

> +
> +	cldev = to_mei_cl_device(dev);
> +
> +	return mei_cldev_send_gsc_command(cldev, client_id, fence_id, sg_in, total_in_len, sg_out);
> +}
> +
>  static const struct i915_pxp_component_ops mei_pxp_ops = {
>  	.owner = THIS_MODULE,
>  	.send = mei_pxp_send_message,
>  	.recv = mei_pxp_receive_message,
> +	.gsc_command = mei_pxp_gsc_command,
>  };
>  
>  static int mei_component_master_bind(struct device *dev)
> diff --git a/include/drm/i915_pxp_tee_interface.h b/include/drm/i915_pxp_tee_interface.h
> index af593ec64469..a702b6ec17f7 100644
> --- a/include/drm/i915_pxp_tee_interface.h
> +++ b/include/drm/i915_pxp_tee_interface.h
> @@ -8,6 +8,7 @@
>  
>  #include <linux/mutex.h>
>  #include <linux/device.h>
> +struct scatterlist;
>  
>  /**
>   * struct i915_pxp_component_ops - ops for PXP services.
> @@ -23,6 +24,10 @@ struct i915_pxp_component_ops {
>  
>  	int (*send)(struct device *dev, const void *message, size_t size);
>  	int (*recv)(struct device *dev, void *buffer, size_t size);
> +	ssize_t (*gsc_command)(struct device *dev, u8 client_id, u32 fence_id,
> +			       struct scatterlist *sg_in, size_t total_in_len,
> +			       struct scatterlist *sg_out);

No documentation for this new callback?

The build should give you are warning :(
Winkler, Tomas Sept. 9, 2022, 6:38 a.m. UTC | #2 
> 
> On Thu, Sep 08, 2022 at 05:16:02PM -0700, Daniele Ceraolo Spurio wrote:
> > +static ssize_t mei_pxp_gsc_command(struct device *dev, u8 client_id,
> u32 fence_id,
> > +				   struct scatterlist *sg_in, size_t total_in_len,
> > +				   struct scatterlist *sg_out)
> > +{
> > +	struct mei_cl_device *cldev;
> > +
> > +	if (!dev || !sg_in || !sg_out)
> > +		return -EINVAL;
> 
> How can these ever be NULL?  Doesn't the core control this, so why would
> that happen?
This is any interface function between modules, I think it is not healthy to take assumptions here about how caller
behaves, this is not an inner functions. This is important even for catching programmatical mistakes. 
> 
> Don't check for things that can never happen.
> 
> > +
> > +	cldev = to_mei_cl_device(dev);
> > +
> > +	return mei_cldev_send_gsc_command(cldev, client_id, fence_id,
> sg_in,
> > +total_in_len, sg_out); }
> > +
> >  static const struct i915_pxp_component_ops mei_pxp_ops = {
> >  	.owner = THIS_MODULE,
> >  	.send = mei_pxp_send_message,
> >  	.recv = mei_pxp_receive_message,
> > +	.gsc_command = mei_pxp_gsc_command,
> >  };
> >
> >  static int mei_component_master_bind(struct device *dev) diff --git
> > a/include/drm/i915_pxp_tee_interface.h
> > b/include/drm/i915_pxp_tee_interface.h
> > index af593ec64469..a702b6ec17f7 100644
> > --- a/include/drm/i915_pxp_tee_interface.h
> > +++ b/include/drm/i915_pxp_tee_interface.h
> > @@ -8,6 +8,7 @@
> >
> >  #include <linux/mutex.h>
> >  #include <linux/device.h>
> > +struct scatterlist;
> >
> >  /**
> >   * struct i915_pxp_component_ops - ops for PXP services.
> > @@ -23,6 +24,10 @@ struct i915_pxp_component_ops {
> >
> >  	int (*send)(struct device *dev, const void *message, size_t size);
> >  	int (*recv)(struct device *dev, void *buffer, size_t size);
> > +	ssize_t (*gsc_command)(struct device *dev, u8 client_id, u32
> fence_id,
> > +			       struct scatterlist *sg_in, size_t total_in_len,
> > +			       struct scatterlist *sg_out);
> 
> No documentation for this new callback?
> 
> The build should give you are warning :(
Will fix.

Thanks
Tomas
Greg KH Sept. 9, 2022, 6:42 a.m. UTC | #3 
On Fri, Sep 09, 2022 at 06:38:33AM +0000, Winkler, Tomas wrote:
> > 
> > On Thu, Sep 08, 2022 at 05:16:02PM -0700, Daniele Ceraolo Spurio wrote:
> > > +static ssize_t mei_pxp_gsc_command(struct device *dev, u8 client_id,
> > u32 fence_id,
> > > +				   struct scatterlist *sg_in, size_t total_in_len,
> > > +				   struct scatterlist *sg_out)
> > > +{
> > > +	struct mei_cl_device *cldev;
> > > +
> > > +	if (!dev || !sg_in || !sg_out)
> > > +		return -EINVAL;
> > 
> > How can these ever be NULL?  Doesn't the core control this, so why would
> > that happen?
> This is any interface function between modules, I think it is not healthy to take assumptions here about how caller
> behaves, this is not an inner functions. This is important even for catching programmatical mistakes. 

It is a static function being called from a framework.  Enforce this in
the framework, don't sprinkle this stuff everywhere, the kernel is NOT
defensive about internal users like this otherwise it will overwhelm
every function call.

thanks,

greg k-h
Winkler, Tomas Sept. 12, 2022, 9:59 a.m. UTC | #4 
> 
> On Fri, Sep 09, 2022 at 06:38:33AM +0000, Winkler, Tomas wrote:
> > >
> > > On Thu, Sep 08, 2022 at 05:16:02PM -0700, Daniele Ceraolo Spurio wrote:
> > > > +static ssize_t mei_pxp_gsc_command(struct device *dev, u8
> > > > +client_id,
> > > u32 fence_id,
> > > > +				   struct scatterlist *sg_in, size_t total_in_len,
> > > > +				   struct scatterlist *sg_out) {
> > > > +	struct mei_cl_device *cldev;
> > > > +
> > > > +	if (!dev || !sg_in || !sg_out)
> > > > +		return -EINVAL;
> > >
> > > How can these ever be NULL?  Doesn't the core control this, so why
> > > would that happen?
> > This is any interface function between modules, I think it is not
> > healthy to take assumptions here about how caller behaves, this is not an
> inner functions. This is important even for catching programmatical mistakes.
> 
> It is a static function being called from a framework.  Enforce this in the
> framework, don't sprinkle this stuff everywhere, the kernel is NOT defensive
> about internal users like this otherwise it will overwhelm every function call.

I'm not sure, this is the case here.  The function is passed to  i915 (other driver) driver via struct i915_pxp_component_op.
This is outside of the component framework. 

Thanks
Tomas
Greg KH Sept. 12, 2022, 2:25 p.m. UTC | #5 
On Mon, Sep 12, 2022 at 09:59:45AM +0000, Winkler, Tomas wrote:
> > 
> > On Fri, Sep 09, 2022 at 06:38:33AM +0000, Winkler, Tomas wrote:
> > > >
> > > > On Thu, Sep 08, 2022 at 05:16:02PM -0700, Daniele Ceraolo Spurio wrote:
> > > > > +static ssize_t mei_pxp_gsc_command(struct device *dev, u8
> > > > > +client_id,
> > > > u32 fence_id,
> > > > > +				   struct scatterlist *sg_in, size_t total_in_len,
> > > > > +				   struct scatterlist *sg_out) {
> > > > > +	struct mei_cl_device *cldev;
> > > > > +
> > > > > +	if (!dev || !sg_in || !sg_out)
> > > > > +		return -EINVAL;
> > > >
> > > > How can these ever be NULL?  Doesn't the core control this, so why
> > > > would that happen?
> > > This is any interface function between modules, I think it is not
> > > healthy to take assumptions here about how caller behaves, this is not an
> > inner functions. This is important even for catching programmatical mistakes.
> > 
> > It is a static function being called from a framework.  Enforce this in the
> > framework, don't sprinkle this stuff everywhere, the kernel is NOT defensive
> > about internal users like this otherwise it will overwhelm every function call.
> 
> I'm not sure, this is the case here.  The function is passed to  i915 (other driver) driver via struct i915_pxp_component_op.
> This is outside of the component framework. 

Then pass in the real mei structure please.  Don't force someone else to
"find" it in the device tree like you are currently trying to do.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/misc/mei/pxp/mei_pxp.c b/drivers/misc/mei/pxp/mei_pxp.c
index 5c39457e3f53..17c5d201603f 100644
--- a/drivers/misc/mei/pxp/mei_pxp.c
+++ b/drivers/misc/mei/pxp/mei_pxp.c
@@ -77,10 +77,38 @@  mei_pxp_receive_message(struct device *dev, void *buffer, size_t size)
 	return byte;
 }
 
+/**
+ * mei_pxp_gsc_command() - sends a gsc command, by sending
+ * a sgl mei message to gsc and receiving reply from gsc
+ *
+ * @dev: device corresponding to the mei_cl_device
+ * @client_id: client id to send the command to
+ * @fence_id: fence id to send the command to
+ * @sg_in: scatter gather list containing addresses for rx message buffer
+ * @total_in_len: total length of data in 'in' sg, can be less than the sum of buffers sizes
+ * @sg_out: scatter gather list containing addresses for tx message buffer
+ *
+ * Return: bytes sent on Success, <0 on Failure
+ */
+static ssize_t mei_pxp_gsc_command(struct device *dev, u8 client_id, u32 fence_id,
+				   struct scatterlist *sg_in, size_t total_in_len,
+				   struct scatterlist *sg_out)
+{
+	struct mei_cl_device *cldev;
+
+	if (!dev || !sg_in || !sg_out)
+		return -EINVAL;
+
+	cldev = to_mei_cl_device(dev);
+
+	return mei_cldev_send_gsc_command(cldev, client_id, fence_id, sg_in, total_in_len, sg_out);
+}
+
 static const struct i915_pxp_component_ops mei_pxp_ops = {
 	.owner = THIS_MODULE,
 	.send = mei_pxp_send_message,
 	.recv = mei_pxp_receive_message,
+	.gsc_command = mei_pxp_gsc_command,
 };
 
 static int mei_component_master_bind(struct device *dev)
diff --git a/include/drm/i915_pxp_tee_interface.h b/include/drm/i915_pxp_tee_interface.h
index af593ec64469..a702b6ec17f7 100644
--- a/include/drm/i915_pxp_tee_interface.h
+++ b/include/drm/i915_pxp_tee_interface.h
@@ -8,6 +8,7 @@ 
 
 #include <linux/mutex.h>
 #include <linux/device.h>
+struct scatterlist;
 
 /**
  * struct i915_pxp_component_ops - ops for PXP services.
@@ -23,6 +24,10 @@  struct i915_pxp_component_ops {
 
 	int (*send)(struct device *dev, const void *message, size_t size);
 	int (*recv)(struct device *dev, void *buffer, size_t size);
+	ssize_t (*gsc_command)(struct device *dev, u8 client_id, u32 fence_id,
+			       struct scatterlist *sg_in, size_t total_in_len,
+			       struct scatterlist *sg_out);
+
 };
 
 /**