Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Jae Hyun Yoo]

On 1/11/2018 1:06 AM, Benjamin Herrenschmidt wrote:

On Tue, 2018-01-09 at 14:31 -0800, Jae Hyun Yoo wrote:

+struct peci_rd_ia_msr_msg {
+   unsigned char target;
+   unsigned char thread_id;
+   unsigned short address;
+   unsigned long value;
+};


Those types are representing messages on the wire ?

In that case those types aren't suitable. For example "long" will have
a different size and alignment for 32 and 64-bit userspace. There are
size-explicit userspace types available.

Also I didn't see any endianness annotations in there. Is that expected
? IE are those wire format packets ?

Cheers,
Ben.



Only the 'peci_xfer_msg' struct is representing messages on the wire. 
All userspace messages which is using other struct definitions will be 
copied into the 'peci_xfer_msg' for each member variable in driver, but 
anyway, type definitions of each member variable should be fixed as you 
said. Will fix it.


Thanks,
Jae

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Jae Hyun Yoo]

On 1/11/2018 1:06 AM, Benjamin Herrenschmidt wrote:

On Tue, 2018-01-09 at 14:31 -0800, Jae Hyun Yoo wrote:

+struct peci_rd_ia_msr_msg {
+   unsigned char target;
+   unsigned char thread_id;
+   unsigned short address;
+   unsigned long value;
+};


Those types are representing messages on the wire ?

In that case those types aren't suitable. For example "long" will have
a different size and alignment for 32 and 64-bit userspace. There are
size-explicit userspace types available.

Also I didn't see any endianness annotations in there. Is that expected
? IE are those wire format packets ?

Cheers,
Ben.



Only the 'peci_xfer_msg' struct is representing messages on the wire. 
All userspace messages which is using other struct definitions will be 
copied into the 'peci_xfer_msg' for each member variable in driver, but 
anyway, type definitions of each member variable should be fixed as you 
said. Will fix it.


Thanks,
Jae

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Jae Hyun Yoo]

On 1/11/2018 1:02 AM, Benjamin Herrenschmidt wrote:

On Wed, 2018-01-10 at 11:18 +0100, Greg KH wrote:

On Tue, Jan 09, 2018 at 02:31:23PM -0800, Jae Hyun Yoo wrote:

This commit adds driver implementation for Aspeed PECI. Also adds
generic peci.h and peci_ioctl.h files to provide compatibility
to peci drivers that can be implemented later e.g. Nuvoton's BMC
SoC family.


We don't add code that could be used "sometime in the future".  Only
include stuff that we use now.

Please fix up this series based on that and resubmit.  There should not
be any need for any uapi file then, right?


No Greg, I think you misunderstood (unless I misread myself).

What Jae means is that since PECI is a standard and other drivers
implementing the same ioctl interface and messages will eventually go
upstream, instead of having the ioctl definitions in a driver specific
locations, they go in a generic spot, as they define a generic API for
all PECI drivers, including the one that's getting merged now.

IE. This doesn't add unused stuff, it just puts the API parts of it
into a generic location.

At least that's my understanding from a, granted cursory, look at the
patch.

That said, I do have a problem with the structure definitions of the
various packet types as they use "long" which has a variable size and
unclear alignment. It should be using __u8, __u16 and __u32...

Cheers,
Ben.



Thanks for your clear explanation. That is what I actually intended to. 
However, the structure definitions you and Greg pointed out need to be 
corrected. I will fix it.


Thanks,
Jae

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Jae Hyun Yoo]

On 1/11/2018 1:02 AM, Benjamin Herrenschmidt wrote:

On Wed, 2018-01-10 at 11:18 +0100, Greg KH wrote:

On Tue, Jan 09, 2018 at 02:31:23PM -0800, Jae Hyun Yoo wrote:

This commit adds driver implementation for Aspeed PECI. Also adds
generic peci.h and peci_ioctl.h files to provide compatibility
to peci drivers that can be implemented later e.g. Nuvoton's BMC
SoC family.


We don't add code that could be used "sometime in the future".  Only
include stuff that we use now.

Please fix up this series based on that and resubmit.  There should not
be any need for any uapi file then, right?


No Greg, I think you misunderstood (unless I misread myself).

What Jae means is that since PECI is a standard and other drivers
implementing the same ioctl interface and messages will eventually go
upstream, instead of having the ioctl definitions in a driver specific
locations, they go in a generic spot, as they define a generic API for
all PECI drivers, including the one that's getting merged now.

IE. This doesn't add unused stuff, it just puts the API parts of it
into a generic location.

At least that's my understanding from a, granted cursory, look at the
patch.

That said, I do have a problem with the structure definitions of the
various packet types as they use "long" which has a variable size and
unclear alignment. It should be using __u8, __u16 and __u32...

Cheers,
Ben.



Thanks for your clear explanation. That is what I actually intended to. 
However, the structure definitions you and Greg pointed out need to be 
corrected. I will fix it.


Thanks,
Jae

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Benjamin Herrenschmidt]

On Tue, 2018-01-09 at 14:31 -0800, Jae Hyun Yoo wrote:
> +struct peci_rd_ia_msr_msg {
> +   unsigned char target;
> +   unsigned char thread_id;
> +   unsigned short address;
> +   unsigned long value;
> +};

Those types are representing messages on the wire ?

In that case those types aren't suitable. For example "long" will have
a different size and alignment for 32 and 64-bit userspace. There are
size-explicit userspace types available.

Also I didn't see any endianness annotations in there. Is that expected
? IE are those wire format packets ?

Cheers,
Ben.

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Benjamin Herrenschmidt]

On Tue, 2018-01-09 at 14:31 -0800, Jae Hyun Yoo wrote:
> +struct peci_rd_ia_msr_msg {
> +   unsigned char target;
> +   unsigned char thread_id;
> +   unsigned short address;
> +   unsigned long value;
> +};

Those types are representing messages on the wire ?

In that case those types aren't suitable. For example "long" will have
a different size and alignment for 32 and 64-bit userspace. There are
size-explicit userspace types available.

Also I didn't see any endianness annotations in there. Is that expected
? IE are those wire format packets ?

Cheers,
Ben.

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Benjamin Herrenschmidt]

On Wed, 2018-01-10 at 11:18 +0100, Greg KH wrote:
> On Tue, Jan 09, 2018 at 02:31:23PM -0800, Jae Hyun Yoo wrote:
> > This commit adds driver implementation for Aspeed PECI. Also adds
> > generic peci.h and peci_ioctl.h files to provide compatibility
> > to peci drivers that can be implemented later e.g. Nuvoton's BMC
> > SoC family.
> 
> We don't add code that could be used "sometime in the future".  Only
> include stuff that we use now.
> 
> Please fix up this series based on that and resubmit.  There should not
> be any need for any uapi file then, right?

No Greg, I think you misunderstood (unless I misread myself).

What Jae means is that since PECI is a standard and other drivers
implementing the same ioctl interface and messages will eventually go
upstream, instead of having the ioctl definitions in a driver specific
locations, they go in a generic spot, as they define a generic API for
all PECI drivers, including the one that's getting merged now.

IE. This doesn't add unused stuff, it just puts the API parts of it
into a generic location.

At least that's my understanding from a, granted cursory, look at the
patch.

That said, I do have a problem with the structure definitions of the
various packet types as they use "long" which has a variable size and
unclear alignment. It should be using __u8, __u16 and __u32...

Cheers,
Ben.

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Benjamin Herrenschmidt]

On Wed, 2018-01-10 at 11:18 +0100, Greg KH wrote:
> On Tue, Jan 09, 2018 at 02:31:23PM -0800, Jae Hyun Yoo wrote:
> > This commit adds driver implementation for Aspeed PECI. Also adds
> > generic peci.h and peci_ioctl.h files to provide compatibility
> > to peci drivers that can be implemented later e.g. Nuvoton's BMC
> > SoC family.
> 
> We don't add code that could be used "sometime in the future".  Only
> include stuff that we use now.
> 
> Please fix up this series based on that and resubmit.  There should not
> be any need for any uapi file then, right?

No Greg, I think you misunderstood (unless I misread myself).

What Jae means is that since PECI is a standard and other drivers
implementing the same ioctl interface and messages will eventually go
upstream, instead of having the ioctl definitions in a driver specific
locations, they go in a generic spot, as they define a generic API for
all PECI drivers, including the one that's getting merged now.

IE. This doesn't add unused stuff, it just puts the API parts of it
into a generic location.

At least that's my understanding from a, granted cursory, look at the
patch.

That said, I do have a problem with the structure definitions of the
various packet types as they use "long" which has a variable size and
unclear alignment. It should be using __u8, __u16 and __u32...

Cheers,
Ben.

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Jae Hyun Yoo]

On 1/10/2018 3:55 AM, Arnd Bergmann wrote:

On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo
 wrote:

This commit adds driver implementation for Aspeed PECI. Also adds
generic peci.h and peci_ioctl.h files to provide compatibility
to peci drivers that can be implemented later e.g. Nuvoton's BMC
SoC family.

Signed-off-by: Jae Hyun Yoo 



+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 


semaphore.h is not used here and can be dropped.



You are right. Will drop it.


+static struct aspeed_peci *aspeed_peci_priv;


Try to avoid instance variables like this one. You should always be able to find
that pointer from whatever structure you were called with.




Okay. I will use driver_data instead.


+   timeout = wait_for_completion_interruptible_timeout(
+   >xfer_complete,
+   msecs_to_jiffies(priv->cmd_timeout_ms));
+
+   dev_dbg(priv->dev, "INT_STS : 0x%08x\n", priv->sts);
+   if (!regmap_read(priv->regmap, AST_PECI_CMD, _state))
+   dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n",
+   PECI_CMD_STS_GET(peci_state));
+   else
+   dev_dbg(priv->dev, "PECI_STATE : read error\n");
+
+   if (timeout <= 0 || !(priv->sts & PECI_INT_CMD_DONE)) {
+   if (timeout <= 0) {
+   dev_dbg(priv->dev, "Timeout waiting for a response!\n");
+   rc = -ETIME;
+   } else {
+   dev_dbg(priv->dev, "No valid response!\n");
+   rc = -EFAULT;
+   }
+   return rc;
+   }


You don't seem to handle -ERESTARTSYS correct here. Either do it
right, or drop the _interruptible part above.



Will add a handling logic for the -ERESTARTSYS.


+typedef int (*ioctl_fn)(struct aspeed_peci *, void *);
+
+static ioctl_fn peci_ioctl_fn[PECI_CMD_MAX] = {
+   ioctl_xfer_msg,
+   ioctl_ping,
+   ioctl_get_dib,
+   ioctl_get_temp,
+   ioctl_rd_pkg_cfg,
+   ioctl_wr_pkg_cfg,
+   ioctl_rd_ia_msr,
+   NULL, /* Reserved */
+   ioctl_rd_pci_cfg,
+   NULL, /* Reserved */
+   ioctl_rd_pci_cfg_local,
+   ioctl_wr_pci_cfg_local,
+};
+
+
+long peci_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+   struct aspeed_peci *priv;
+   long ret = 0;
+   void __user *argp = (void __user *)arg;
+   int timeout = PECI_IDLE_CHECK_TIMEOUT;
+   u8 msg[sizeof(struct peci_xfer_msg)];
+   unsigned int peci_cmd, msg_size;
+   u32 cmd_sts;
+
+   /*
+* Treat it as an inter module call when filp is null but only in case
+* the private data is initialized.
+*/
+   if (filp)
+   priv = container_of(filp->private_data,
+   struct aspeed_peci, miscdev);
+   else
+   priv = aspeed_peci_priv;


Drop this.



peci_ioctl is being called from peci_hwmon as an inter-module call so it 
is needed, but as you suggested in the other patch, I'll consider 
redesign it with adding a peci device class.



+   if (!priv)
+   return -ENXIO;
+
+   switch (cmd) {
+   case PECI_IOC_XFER:
+   case PECI_IOC_PING:
+   case PECI_IOC_GET_DIB:
+   case PECI_IOC_GET_TEMP:
+   case PECI_IOC_RD_PKG_CFG:
+   case PECI_IOC_WR_PKG_CFG:
+   case PECI_IOC_RD_IA_MSR:
+   case PECI_IOC_RD_PCI_CFG:
+   case PECI_IOC_RD_PCI_CFG_LOCAL:
+   case PECI_IOC_WR_PCI_CFG_LOCAL:
+   peci_cmd = _IOC_TYPE(cmd) - PECI_IOC_BASE;
+   msg_size = _IOC_SIZE(cmd);
+   break;


Having to keep the switch() statement and the array above seems a
little fragile. Can you just do one or the other?

Regarding the command set, you have both a low-level PECI_IOC_XFER
interface and a high-level interface. Can you explain why? I'd think that
generally speaking it's better to have only one of the two.



I was intended to provide generic peci command set, also the low level 
PECI_IOC_XFER to provide flexibility for a case when compose a custom 
peci command which cannot be covered by the high-level command set. As 
you said, all other commands can be implemented in the upper layer but 
the benefit of when this driver has the implementation is, it's easy to 
manage retry logic since peci is retrial based protocol intends to do 
not disturb a CPU if the CPU is doing more important task.


However, your thought also makes sense. I'll check the spec again 
whether the high-level command set can cover all cases. If so, I'll 
remove the low-level command.



+   /* Check command sts and bus idle state */
+   while (!regmap_read(priv->regmap, AST_PECI_CMD, _sts)
+  && (cmd_sts & (PECI_CMD_STS_MASK | PECI_CMD_PIN_MON))) {
+   

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Jae Hyun Yoo]

On 1/10/2018 3:55 AM, Arnd Bergmann wrote:

On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo
 wrote:

This commit adds driver implementation for Aspeed PECI. Also adds
generic peci.h and peci_ioctl.h files to provide compatibility
to peci drivers that can be implemented later e.g. Nuvoton's BMC
SoC family.

Signed-off-by: Jae Hyun Yoo 



+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 


semaphore.h is not used here and can be dropped.



You are right. Will drop it.


+static struct aspeed_peci *aspeed_peci_priv;


Try to avoid instance variables like this one. You should always be able to find
that pointer from whatever structure you were called with.




Okay. I will use driver_data instead.


+   timeout = wait_for_completion_interruptible_timeout(
+   >xfer_complete,
+   msecs_to_jiffies(priv->cmd_timeout_ms));
+
+   dev_dbg(priv->dev, "INT_STS : 0x%08x\n", priv->sts);
+   if (!regmap_read(priv->regmap, AST_PECI_CMD, _state))
+   dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n",
+   PECI_CMD_STS_GET(peci_state));
+   else
+   dev_dbg(priv->dev, "PECI_STATE : read error\n");
+
+   if (timeout <= 0 || !(priv->sts & PECI_INT_CMD_DONE)) {
+   if (timeout <= 0) {
+   dev_dbg(priv->dev, "Timeout waiting for a response!\n");
+   rc = -ETIME;
+   } else {
+   dev_dbg(priv->dev, "No valid response!\n");
+   rc = -EFAULT;
+   }
+   return rc;
+   }


You don't seem to handle -ERESTARTSYS correct here. Either do it
right, or drop the _interruptible part above.



Will add a handling logic for the -ERESTARTSYS.


+typedef int (*ioctl_fn)(struct aspeed_peci *, void *);
+
+static ioctl_fn peci_ioctl_fn[PECI_CMD_MAX] = {
+   ioctl_xfer_msg,
+   ioctl_ping,
+   ioctl_get_dib,
+   ioctl_get_temp,
+   ioctl_rd_pkg_cfg,
+   ioctl_wr_pkg_cfg,
+   ioctl_rd_ia_msr,
+   NULL, /* Reserved */
+   ioctl_rd_pci_cfg,
+   NULL, /* Reserved */
+   ioctl_rd_pci_cfg_local,
+   ioctl_wr_pci_cfg_local,
+};
+
+
+long peci_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+   struct aspeed_peci *priv;
+   long ret = 0;
+   void __user *argp = (void __user *)arg;
+   int timeout = PECI_IDLE_CHECK_TIMEOUT;
+   u8 msg[sizeof(struct peci_xfer_msg)];
+   unsigned int peci_cmd, msg_size;
+   u32 cmd_sts;
+
+   /*
+* Treat it as an inter module call when filp is null but only in case
+* the private data is initialized.
+*/
+   if (filp)
+   priv = container_of(filp->private_data,
+   struct aspeed_peci, miscdev);
+   else
+   priv = aspeed_peci_priv;


Drop this.



peci_ioctl is being called from peci_hwmon as an inter-module call so it 
is needed, but as you suggested in the other patch, I'll consider 
redesign it with adding a peci device class.



+   if (!priv)
+   return -ENXIO;
+
+   switch (cmd) {
+   case PECI_IOC_XFER:
+   case PECI_IOC_PING:
+   case PECI_IOC_GET_DIB:
+   case PECI_IOC_GET_TEMP:
+   case PECI_IOC_RD_PKG_CFG:
+   case PECI_IOC_WR_PKG_CFG:
+   case PECI_IOC_RD_IA_MSR:
+   case PECI_IOC_RD_PCI_CFG:
+   case PECI_IOC_RD_PCI_CFG_LOCAL:
+   case PECI_IOC_WR_PCI_CFG_LOCAL:
+   peci_cmd = _IOC_TYPE(cmd) - PECI_IOC_BASE;
+   msg_size = _IOC_SIZE(cmd);
+   break;


Having to keep the switch() statement and the array above seems a
little fragile. Can you just do one or the other?

Regarding the command set, you have both a low-level PECI_IOC_XFER
interface and a high-level interface. Can you explain why? I'd think that
generally speaking it's better to have only one of the two.



I was intended to provide generic peci command set, also the low level 
PECI_IOC_XFER to provide flexibility for a case when compose a custom 
peci command which cannot be covered by the high-level command set. As 
you said, all other commands can be implemented in the upper layer but 
the benefit of when this driver has the implementation is, it's easy to 
manage retry logic since peci is retrial based protocol intends to do 
not disturb a CPU if the CPU is doing more important task.


However, your thought also makes sense. I'll check the spec again 
whether the high-level command set can cover all cases. If so, I'll 
remove the low-level command.



+   /* Check command sts and bus idle state */
+   while (!regmap_read(priv->regmap, AST_PECI_CMD, _sts)
+  && (cmd_sts & (PECI_CMD_STS_MASK | PECI_CMD_PIN_MON))) {
+   if (timeout-- < 0) {
+   

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Jae Hyun Yoo]

On 1/10/2018 2:20 AM, Greg KH wrote:

On Tue, Jan 09, 2018 at 02:31:23PM -0800, Jae Hyun Yoo wrote:

+#pragma pack(push, 1)
+struct peci_xfer_msg {
+   unsigned char client_addr;
+   unsigned char tx_len;
+   unsigned char rx_len;
+   unsigned char tx_buf[MAX_BUFFER_SIZE];
+   unsigned char rx_buf[MAX_BUFFER_SIZE];
+};
+#pragma pack(pop)


For any structure that crosses the user/kernel boundry, you _HAVE_ to
use the "__" variant.  So for here you would use __u8 instead of
"unsigned char" in order for things to work properly.

I'm guessing you didn't test this all out on a mixed 32/64 bit system?

Please fix up and test to ensure that it all works properly before
resubmitting.

thanks,

greg k-h



Thanks for your pointing it out. I'll fix this.

Thanks a lot,
Jae

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Jae Hyun Yoo]

On 1/10/2018 2:20 AM, Greg KH wrote:

On Tue, Jan 09, 2018 at 02:31:23PM -0800, Jae Hyun Yoo wrote:

+#pragma pack(push, 1)
+struct peci_xfer_msg {
+   unsigned char client_addr;
+   unsigned char tx_len;
+   unsigned char rx_len;
+   unsigned char tx_buf[MAX_BUFFER_SIZE];
+   unsigned char rx_buf[MAX_BUFFER_SIZE];
+};
+#pragma pack(pop)


For any structure that crosses the user/kernel boundry, you _HAVE_ to
use the "__" variant.  So for here you would use __u8 instead of
"unsigned char" in order for things to work properly.

I'm guessing you didn't test this all out on a mixed 32/64 bit system?

Please fix up and test to ensure that it all works properly before
resubmitting.

thanks,

greg k-h



Thanks for your pointing it out. I'll fix this.

Thanks a lot,
Jae

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Jae Hyun Yoo]

On 1/10/2018 2:18 AM, Greg KH wrote:

On Tue, Jan 09, 2018 at 02:31:23PM -0800, Jae Hyun Yoo wrote:

This commit adds driver implementation for Aspeed PECI. Also adds
generic peci.h and peci_ioctl.h files to provide compatibility
to peci drivers that can be implemented later e.g. Nuvoton's BMC
SoC family.


We don't add code that could be used "sometime in the future".  Only
include stuff that we use now.

Please fix up this series based on that and resubmit.  There should not
be any need for any uapi file then, right?

thanks,

greg k-h



These header files are being used in this patch set as well. I meant, 
these files also can be used for the future implementation to provide 
compatibility. I will update the commit message.


Thanks,
Jae

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Jae Hyun Yoo]

On 1/10/2018 2:18 AM, Greg KH wrote:

On Tue, Jan 09, 2018 at 02:31:23PM -0800, Jae Hyun Yoo wrote:

This commit adds driver implementation for Aspeed PECI. Also adds
generic peci.h and peci_ioctl.h files to provide compatibility
to peci drivers that can be implemented later e.g. Nuvoton's BMC
SoC family.


We don't add code that could be used "sometime in the future".  Only
include stuff that we use now.

Please fix up this series based on that and resubmit.  There should not
be any need for any uapi file then, right?

thanks,

greg k-h



These header files are being used in this patch set as well. I meant, 
these files also can be used for the future implementation to provide 
compatibility. I will update the commit message.


Thanks,
Jae

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Arnd Bergmann]

On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo
 wrote:
> This commit adds driver implementation for Aspeed PECI. Also adds
> generic peci.h and peci_ioctl.h files to provide compatibility
> to peci drivers that can be implemented later e.g. Nuvoton's BMC
> SoC family.
>
> Signed-off-by: Jae Hyun Yoo 

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

semaphore.h is not used here and can be dropped.

> +static struct aspeed_peci *aspeed_peci_priv;

Try to avoid instance variables like this one. You should always be able to find
that pointer from whatever structure you were called with.


> +   timeout = wait_for_completion_interruptible_timeout(
> +   >xfer_complete,
> +   
> msecs_to_jiffies(priv->cmd_timeout_ms));
> +
> +   dev_dbg(priv->dev, "INT_STS : 0x%08x\n", priv->sts);
> +   if (!regmap_read(priv->regmap, AST_PECI_CMD, _state))
> +   dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n",
> +   PECI_CMD_STS_GET(peci_state));
> +   else
> +   dev_dbg(priv->dev, "PECI_STATE : read error\n");
> +
> +   if (timeout <= 0 || !(priv->sts & PECI_INT_CMD_DONE)) {
> +   if (timeout <= 0) {
> +   dev_dbg(priv->dev, "Timeout waiting for a 
> response!\n");
> +   rc = -ETIME;
> +   } else {
> +   dev_dbg(priv->dev, "No valid response!\n");
> +   rc = -EFAULT;
> +   }
> +   return rc;
> +   }

You don't seem to handle -ERESTARTSYS correct here. Either do it
right, or drop the _interruptible part above.

> +typedef int (*ioctl_fn)(struct aspeed_peci *, void *);
> +
> +static ioctl_fn peci_ioctl_fn[PECI_CMD_MAX] = {
> +   ioctl_xfer_msg,
> +   ioctl_ping,
> +   ioctl_get_dib,
> +   ioctl_get_temp,
> +   ioctl_rd_pkg_cfg,
> +   ioctl_wr_pkg_cfg,
> +   ioctl_rd_ia_msr,
> +   NULL, /* Reserved */
> +   ioctl_rd_pci_cfg,
> +   NULL, /* Reserved */
> +   ioctl_rd_pci_cfg_local,
> +   ioctl_wr_pci_cfg_local,
> +};
> +
> +
> +long peci_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> +   struct aspeed_peci *priv;
> +   long ret = 0;
> +   void __user *argp = (void __user *)arg;
> +   int timeout = PECI_IDLE_CHECK_TIMEOUT;
> +   u8 msg[sizeof(struct peci_xfer_msg)];
> +   unsigned int peci_cmd, msg_size;
> +   u32 cmd_sts;
> +
> +   /*
> +* Treat it as an inter module call when filp is null but only in case
> +* the private data is initialized.
> +*/
> +   if (filp)
> +   priv = container_of(filp->private_data,
> +   struct aspeed_peci, miscdev);
> +   else
> +   priv = aspeed_peci_priv;

Drop this.

> +   if (!priv)
> +   return -ENXIO;
> +
> +   switch (cmd) {
> +   case PECI_IOC_XFER:
> +   case PECI_IOC_PING:
> +   case PECI_IOC_GET_DIB:
> +   case PECI_IOC_GET_TEMP:
> +   case PECI_IOC_RD_PKG_CFG:
> +   case PECI_IOC_WR_PKG_CFG:
> +   case PECI_IOC_RD_IA_MSR:
> +   case PECI_IOC_RD_PCI_CFG:
> +   case PECI_IOC_RD_PCI_CFG_LOCAL:
> +   case PECI_IOC_WR_PCI_CFG_LOCAL:
> +   peci_cmd = _IOC_TYPE(cmd) - PECI_IOC_BASE;
> +   msg_size = _IOC_SIZE(cmd);
> +   break;

Having to keep the switch() statement and the array above seems a
little fragile. Can you just do one or the other?

Regarding the command set, you have both a low-level PECI_IOC_XFER
interface and a high-level interface. Can you explain why? I'd think that
generally speaking it's better to have only one of the two.

> +   /* Check command sts and bus idle state */
> +   while (!regmap_read(priv->regmap, AST_PECI_CMD, _sts)
> +  && (cmd_sts & (PECI_CMD_STS_MASK | PECI_CMD_PIN_MON))) {
> +   if (timeout-- < 0) {
> +   dev_dbg(priv->dev, "Timeout waiting for idle 
> state!\n");
> +   ret = -ETIME;
> +   goto out;
> +   }
> +   usleep_range(1, 11000);
> +   };

To implement timeout, it's better to replace the counter with a
jiffies/time_before or ktime_get()/ktime_before() check, since usleep_range()
is might sleep considerably longer than expected.

> +EXPORT_SYMBOL_GPL(peci_ioctl);

No user of this, so drop it.

> +static int aspeed_peci_open(struct inode *inode, struct file *filp)
> +{
> +   struct aspeed_peci *priv =
> +   container_of(filp->private_data, struct aspeed_peci, miscdev);
> +
> +   atomic_inc(>ref_count);
> +
> +   dev_dbg(priv->dev, "ref_count : %d\n", 

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Arnd Bergmann]

On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo
 wrote:
> This commit adds driver implementation for Aspeed PECI. Also adds
> generic peci.h and peci_ioctl.h files to provide compatibility
> to peci drivers that can be implemented later e.g. Nuvoton's BMC
> SoC family.
>
> Signed-off-by: Jae Hyun Yoo 

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

semaphore.h is not used here and can be dropped.

> +static struct aspeed_peci *aspeed_peci_priv;

Try to avoid instance variables like this one. You should always be able to find
that pointer from whatever structure you were called with.


> +   timeout = wait_for_completion_interruptible_timeout(
> +   >xfer_complete,
> +   
> msecs_to_jiffies(priv->cmd_timeout_ms));
> +
> +   dev_dbg(priv->dev, "INT_STS : 0x%08x\n", priv->sts);
> +   if (!regmap_read(priv->regmap, AST_PECI_CMD, _state))
> +   dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n",
> +   PECI_CMD_STS_GET(peci_state));
> +   else
> +   dev_dbg(priv->dev, "PECI_STATE : read error\n");
> +
> +   if (timeout <= 0 || !(priv->sts & PECI_INT_CMD_DONE)) {
> +   if (timeout <= 0) {
> +   dev_dbg(priv->dev, "Timeout waiting for a 
> response!\n");
> +   rc = -ETIME;
> +   } else {
> +   dev_dbg(priv->dev, "No valid response!\n");
> +   rc = -EFAULT;
> +   }
> +   return rc;
> +   }

You don't seem to handle -ERESTARTSYS correct here. Either do it
right, or drop the _interruptible part above.

> +typedef int (*ioctl_fn)(struct aspeed_peci *, void *);
> +
> +static ioctl_fn peci_ioctl_fn[PECI_CMD_MAX] = {
> +   ioctl_xfer_msg,
> +   ioctl_ping,
> +   ioctl_get_dib,
> +   ioctl_get_temp,
> +   ioctl_rd_pkg_cfg,
> +   ioctl_wr_pkg_cfg,
> +   ioctl_rd_ia_msr,
> +   NULL, /* Reserved */
> +   ioctl_rd_pci_cfg,
> +   NULL, /* Reserved */
> +   ioctl_rd_pci_cfg_local,
> +   ioctl_wr_pci_cfg_local,
> +};
> +
> +
> +long peci_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> +   struct aspeed_peci *priv;
> +   long ret = 0;
> +   void __user *argp = (void __user *)arg;
> +   int timeout = PECI_IDLE_CHECK_TIMEOUT;
> +   u8 msg[sizeof(struct peci_xfer_msg)];
> +   unsigned int peci_cmd, msg_size;
> +   u32 cmd_sts;
> +
> +   /*
> +* Treat it as an inter module call when filp is null but only in case
> +* the private data is initialized.
> +*/
> +   if (filp)
> +   priv = container_of(filp->private_data,
> +   struct aspeed_peci, miscdev);
> +   else
> +   priv = aspeed_peci_priv;

Drop this.

> +   if (!priv)
> +   return -ENXIO;
> +
> +   switch (cmd) {
> +   case PECI_IOC_XFER:
> +   case PECI_IOC_PING:
> +   case PECI_IOC_GET_DIB:
> +   case PECI_IOC_GET_TEMP:
> +   case PECI_IOC_RD_PKG_CFG:
> +   case PECI_IOC_WR_PKG_CFG:
> +   case PECI_IOC_RD_IA_MSR:
> +   case PECI_IOC_RD_PCI_CFG:
> +   case PECI_IOC_RD_PCI_CFG_LOCAL:
> +   case PECI_IOC_WR_PCI_CFG_LOCAL:
> +   peci_cmd = _IOC_TYPE(cmd) - PECI_IOC_BASE;
> +   msg_size = _IOC_SIZE(cmd);
> +   break;

Having to keep the switch() statement and the array above seems a
little fragile. Can you just do one or the other?

Regarding the command set, you have both a low-level PECI_IOC_XFER
interface and a high-level interface. Can you explain why? I'd think that
generally speaking it's better to have only one of the two.

> +   /* Check command sts and bus idle state */
> +   while (!regmap_read(priv->regmap, AST_PECI_CMD, _sts)
> +  && (cmd_sts & (PECI_CMD_STS_MASK | PECI_CMD_PIN_MON))) {
> +   if (timeout-- < 0) {
> +   dev_dbg(priv->dev, "Timeout waiting for idle 
> state!\n");
> +   ret = -ETIME;
> +   goto out;
> +   }
> +   usleep_range(1, 11000);
> +   };

To implement timeout, it's better to replace the counter with a
jiffies/time_before or ktime_get()/ktime_before() check, since usleep_range()
is might sleep considerably longer than expected.

> +EXPORT_SYMBOL_GPL(peci_ioctl);

No user of this, so drop it.

> +static int aspeed_peci_open(struct inode *inode, struct file *filp)
> +{
> +   struct aspeed_peci *priv =
> +   container_of(filp->private_data, struct aspeed_peci, miscdev);
> +
> +   atomic_inc(>ref_count);
> +
> +   dev_dbg(priv->dev, "ref_count : %d\n", atomic_read(>ref_count));
> +
> +   return 0;
> +}
> +
> 

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Greg KH]

On Tue, Jan 09, 2018 at 02:31:23PM -0800, Jae Hyun Yoo wrote:
> +#pragma pack(push, 1)
> +struct peci_xfer_msg {
> + unsigned char client_addr;
> + unsigned char tx_len;
> + unsigned char rx_len;
> + unsigned char tx_buf[MAX_BUFFER_SIZE];
> + unsigned char rx_buf[MAX_BUFFER_SIZE];
> +};
> +#pragma pack(pop)

For any structure that crosses the user/kernel boundry, you _HAVE_ to
use the "__" variant.  So for here you would use __u8 instead of
"unsigned char" in order for things to work properly.

I'm guessing you didn't test this all out on a mixed 32/64 bit system?

Please fix up and test to ensure that it all works properly before
resubmitting.

thanks,

greg k-h

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Greg KH]

On Tue, Jan 09, 2018 at 02:31:23PM -0800, Jae Hyun Yoo wrote:
> +#pragma pack(push, 1)
> +struct peci_xfer_msg {
> + unsigned char client_addr;
> + unsigned char tx_len;
> + unsigned char rx_len;
> + unsigned char tx_buf[MAX_BUFFER_SIZE];
> + unsigned char rx_buf[MAX_BUFFER_SIZE];
> +};
> +#pragma pack(pop)

For any structure that crosses the user/kernel boundry, you _HAVE_ to
use the "__" variant.  So for here you would use __u8 instead of
"unsigned char" in order for things to work properly.

I'm guessing you didn't test this all out on a mixed 32/64 bit system?

Please fix up and test to ensure that it all works properly before
resubmitting.

thanks,

greg k-h

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Greg KH]

On Tue, Jan 09, 2018 at 02:31:23PM -0800, Jae Hyun Yoo wrote:
> This commit adds driver implementation for Aspeed PECI. Also adds
> generic peci.h and peci_ioctl.h files to provide compatibility
> to peci drivers that can be implemented later e.g. Nuvoton's BMC
> SoC family.

We don't add code that could be used "sometime in the future".  Only
include stuff that we use now.

Please fix up this series based on that and resubmit.  There should not
be any need for any uapi file then, right?

thanks,

greg k-h

Re: [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers

[Greg KH]

On Tue, Jan 09, 2018 at 02:31:23PM -0800, Jae Hyun Yoo wrote:
> This commit adds driver implementation for Aspeed PECI. Also adds
> generic peci.h and peci_ioctl.h files to provide compatibility
> to peci drivers that can be implemented later e.g. Nuvoton's BMC
> SoC family.

We don't add code that could be used "sometime in the future".  Only
include stuff that we use now.

Please fix up this series based on that and resubmit.  There should not
be any need for any uapi file then, right?

thanks,

greg k-h