Re: [PATCH v2 1/8] [PATCH 1/8] drivers/peci: Add support for PECI bus driver core

[Jae Hyun Yoo]

Hi Julia,

Thanks for sharing your time on reviewing it. Please see my inline answers.

Jae

On 3/6/2018 7:19 PM, Julia Cartwright wrote:

On Wed, Feb 21, 2018 at 08:15:59AM -0800, Jae Hyun Yoo wrote:

This commit adds driver implementation for PECI bus into linux
driver framework.

Signed-off-by: Jae Hyun Yoo 
---

[..]

+static int peci_locked_xfer(struct peci_adapter *adapter,
+   struct peci_xfer_msg *msg,
+   bool do_retry,
+   bool has_aw_fcs)


_locked generally means that this function is invoked with some critical
lock held, what lock does the caller need to acquire before invoking
this function?



I intended to show that this function has a mutex locking inside for 
serialization of PECI data transactions from multiple callers, but as 
you commented out below, the mutex protection scope should be adjusted 
to make that covers the peci_scan_cmd_mask() function too. I'll rewrite 
the mutex protection scope then this function will be in the locked scope.



+{
+   ktime_t start, end;
+   s64 elapsed_ms;
+   int rc = 0;
+
+   if (!adapter->xfer) {


Is this really an optional feature of an adapter?  If this is not
optional, then this check should be in place when the adapter is
registered, not here.  (And it should WARN_ON(), because it's a driver
developer error).



I agree with you. I'll move this code into the peci_register_adapter() 
function.



+   dev_dbg(>dev, "PECI level transfers not supported\n");
+   return -ENODEV;
+   }
+
+   if (in_atomic() || irqs_disabled()) {


As Andrew mentioned, this is broken.

You don't even need a might_sleep().  The locking functions you use here
will already include a might_sleep() w/ CONFIG_DEBUG_ATOMIC_SLEEP.



Thanks for letting me know that. I'll drop that checking code and 
might_sleep() too.



+   rt_mutex_trylock(>bus_lock);
+   if (!rc)
+   return -EAGAIN; /* PECI activity is ongoing */
+   } else {
+   rt_mutex_lock(>bus_lock);
+   }
+
+   if (do_retry)
+   start = ktime_get();
+
+   do {
+   rc = adapter->xfer(adapter, msg);
+
+   if (!do_retry)
+   break;
+
+   /* Per the PECI spec, need to retry commands that return 0x8x */
+   if (!(!rc && ((msg->rx_buf[0] & DEV_PECI_CC_RETRY_ERR_MASK) ==
+ DEV_PECI_CC_TIMEOUT)))
+   break;


This is pretty difficult to parse.  Can you split it into two different
conditions?



Sure. I'll split it out.


+
+   /* Set the retry bit to indicate a retry attempt */
+   msg->tx_buf[1] |= DEV_PECI_RETRY_BIT;


Are you sure this bit is to be set in the _second_ byte of tx_buf?



Yes, I'm pretty sure. The first byte contains a PECI command value and 
the second byte contains 'HostID[7:1] & Retry[0]' value.



+
+   /* Recalculate the AW FCS if it has one */
+   if (has_aw_fcs)
+   msg->tx_buf[msg->tx_len - 1] = 0x80 ^
+   peci_aw_fcs((u8 *)msg,
+   2 + msg->tx_len);
+
+   /* Retry for at least 250ms before returning an error */
+   end = ktime_get();
+   elapsed_ms = ktime_to_ms(ktime_sub(end, start));
+   if (elapsed_ms >= DEV_PECI_RETRY_TIME_MS) {
+   dev_dbg(>dev, "Timeout retrying xfer!\n");
+   break;
+   }
+   } while (true);
+
+   rt_mutex_unlock(>bus_lock);
+
+   return rc;
+}
+
+static int peci_xfer(struct peci_adapter *adapter, struct peci_xfer_msg *msg)
+{
+   return peci_locked_xfer(adapter, msg, false, false);
+}
+
+static int peci_xfer_with_retries(struct peci_adapter *adapter,
+ struct peci_xfer_msg *msg,
+ bool has_aw_fcs)
+{
+   return peci_locked_xfer(adapter, msg, true, has_aw_fcs);
+}
+
+static int peci_scan_cmd_mask(struct peci_adapter *adapter)
+{
+   struct peci_xfer_msg msg;
+   u32 dib;
+   int rc = 0;
+
+   /* Update command mask just once */
+   if (adapter->cmd_mask & BIT(PECI_CMD_PING))
+   return 0;
+
+   msg.addr  = PECI_BASE_ADDR;
+   msg.tx_len= GET_DIB_WR_LEN;
+   msg.rx_len= GET_DIB_RD_LEN;
+   msg.tx_buf[0] = GET_DIB_PECI_CMD;
+
+   rc = peci_xfer(adapter, );
+   if (rc < 0) {
+   dev_dbg(>dev, "PECI xfer error, rc : %d\n", rc);
+   return rc;
+   }
+
+   dib = msg.rx_buf[0] | (msg.rx_buf[1] << 8) |
+ (msg.rx_buf[2] << 16) | (msg.rx_buf[3] << 24);
+
+   /* Check special case for Get DIB command */
+   if (dib == 0x00) {
+   dev_dbg(>dev, "DIB read as 0x00\n");

Re: [PATCH v2 1/8] [PATCH 1/8] drivers/peci: Add support for PECI bus driver core

[Julia Cartwright]

On Wed, Feb 21, 2018 at 08:15:59AM -0800, Jae Hyun Yoo wrote:
> This commit adds driver implementation for PECI bus into linux
> driver framework.
> 
> Signed-off-by: Jae Hyun Yoo 
> ---
[..]
> +static int peci_locked_xfer(struct peci_adapter *adapter,
> + struct peci_xfer_msg *msg,
> + bool do_retry,
> + bool has_aw_fcs)

_locked generally means that this function is invoked with some critical
lock held, what lock does the caller need to acquire before invoking
this function?

> +{
> + ktime_t start, end;
> + s64 elapsed_ms;
> + int rc = 0;
> +
> + if (!adapter->xfer) {

Is this really an optional feature of an adapter?  If this is not
optional, then this check should be in place when the adapter is
registered, not here.  (And it should WARN_ON(), because it's a driver
developer error).

> + dev_dbg(>dev, "PECI level transfers not supported\n");
> + return -ENODEV;
> + }
> +
> + if (in_atomic() || irqs_disabled()) {

As Andrew mentioned, this is broken.

You don't even need a might_sleep().  The locking functions you use here
will already include a might_sleep() w/ CONFIG_DEBUG_ATOMIC_SLEEP.

> + rt_mutex_trylock(>bus_lock);
> + if (!rc)
> + return -EAGAIN; /* PECI activity is ongoing */
> + } else {
> + rt_mutex_lock(>bus_lock);
> + }
> +
> + if (do_retry)
> + start = ktime_get();
> +
> + do {
> + rc = adapter->xfer(adapter, msg);
> +
> + if (!do_retry)
> + break;
> +
> + /* Per the PECI spec, need to retry commands that return 0x8x */
> + if (!(!rc && ((msg->rx_buf[0] & DEV_PECI_CC_RETRY_ERR_MASK) ==
> +   DEV_PECI_CC_TIMEOUT)))
> + break;

This is pretty difficult to parse.  Can you split it into two different
conditions?

> +
> + /* Set the retry bit to indicate a retry attempt */
> + msg->tx_buf[1] |= DEV_PECI_RETRY_BIT;

Are you sure this bit is to be set in the _second_ byte of tx_buf?

> +
> + /* Recalculate the AW FCS if it has one */
> + if (has_aw_fcs)
> + msg->tx_buf[msg->tx_len - 1] = 0x80 ^
> + peci_aw_fcs((u8 *)msg,
> + 2 + msg->tx_len);
> +
> + /* Retry for at least 250ms before returning an error */
> + end = ktime_get();
> + elapsed_ms = ktime_to_ms(ktime_sub(end, start));
> + if (elapsed_ms >= DEV_PECI_RETRY_TIME_MS) {
> + dev_dbg(>dev, "Timeout retrying xfer!\n");
> + break;
> + }
> + } while (true);
> +
> + rt_mutex_unlock(>bus_lock);
> +
> + return rc;
> +}
> +
> +static int peci_xfer(struct peci_adapter *adapter, struct peci_xfer_msg *msg)
> +{
> + return peci_locked_xfer(adapter, msg, false, false);
> +}
> +
> +static int peci_xfer_with_retries(struct peci_adapter *adapter,
> +   struct peci_xfer_msg *msg,
> +   bool has_aw_fcs)
> +{
> + return peci_locked_xfer(adapter, msg, true, has_aw_fcs);
> +}
> +
> +static int peci_scan_cmd_mask(struct peci_adapter *adapter)
> +{
> + struct peci_xfer_msg msg;
> + u32 dib;
> + int rc = 0;
> +
> + /* Update command mask just once */
> + if (adapter->cmd_mask & BIT(PECI_CMD_PING))
> + return 0;
> +
> + msg.addr  = PECI_BASE_ADDR;
> + msg.tx_len= GET_DIB_WR_LEN;
> + msg.rx_len= GET_DIB_RD_LEN;
> + msg.tx_buf[0] = GET_DIB_PECI_CMD;
> +
> + rc = peci_xfer(adapter, );
> + if (rc < 0) {
> + dev_dbg(>dev, "PECI xfer error, rc : %d\n", rc);
> + return rc;
> + }
> +
> + dib = msg.rx_buf[0] | (msg.rx_buf[1] << 8) |
> +   (msg.rx_buf[2] << 16) | (msg.rx_buf[3] << 24);
> +
> + /* Check special case for Get DIB command */
> + if (dib == 0x00) {
> + dev_dbg(>dev, "DIB read as 0x00\n");
> + return -1;
> + }
> +
> + if (!rc) {

You should change this to:

if (rc) {
dev_dbg(>dev, "Error reading DIB, rc : %d\n", rc);
return rc;
}

And then leave the happy path below unindented.

> + /**
> +  * setting up the supporting commands based on minor rev#
> +  * see PECI Spec Table 3-1
> +  */
> + dib = (dib >> 8) & 0xF;
> +
> + if (dib >= 0x1) {
> + adapter->cmd_mask |= BIT(PECI_CMD_RD_PKG_CFG);
> + adapter->cmd_mask |= BIT(PECI_CMD_WR_PKG_CFG);
> + }
> +
> + if (dib >= 0x2)
> + adapter->cmd_mask |= BIT(PECI_CMD_RD_IA_MSR);
> +
> + if 

Re: [PATCH v2 1/8] [PATCH 1/8] drivers/peci: Add support for PECI bus driver core

[Jae Hyun Yoo]

On 2/21/2018 10:54 PM, Greg KH wrote:

On Wed, Feb 21, 2018 at 12:42:30PM -0800, Jae Hyun Yoo wrote:

On 2/21/2018 9:58 AM, Greg KH wrote:

On Wed, Feb 21, 2018 at 08:15:59AM -0800, Jae Hyun Yoo wrote:

This commit adds driver implementation for PECI bus into linux
driver framework.

Signed-off-by: Jae Hyun Yoo 
---


Why is there no other Intel developers willing to review and sign off on
this patch?  Please get their review first before asking us to do their
work for them :)

thanks,

greg k-h



Hi Greg,

This patch set got our internal review process. Sorry if it's code quality
is under your expectation but it's the reason why I'm asking you to review
the code. Could you please share your time to review it?


Nope.  If no other Intel developer thinks it is good enough to put their
name on it as part of their review process, why should I?

Again, please use the resources you have, to fix the obvious problems in
your code, BEFORE asking the community to do that work for you.

greg k-h



Okay. I'll take our internal review process again on this patch set and 
collect more credit tags before submitting v3.


Thanks for your advice!

Jae
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/8] [PATCH 1/8] drivers/peci: Add support for PECI bus driver core

[kbuild test robot]

Hi Jae,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v4.16-rc2 next-20180221]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Jae-Hyun-Yoo/PECI-device-driver-introduction/20180222-054545
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/peci/peci-core.c:773:29: sparse: symbol 'peci_match_id' was not 
>> declared. Should it be

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/8] [PATCH 1/8] drivers/peci: Add support for PECI bus driver core

[Greg KH]

On Wed, Feb 21, 2018 at 12:42:30PM -0800, Jae Hyun Yoo wrote:
> On 2/21/2018 9:58 AM, Greg KH wrote:
> > On Wed, Feb 21, 2018 at 08:15:59AM -0800, Jae Hyun Yoo wrote:
> > > This commit adds driver implementation for PECI bus into linux
> > > driver framework.
> > > 
> > > Signed-off-by: Jae Hyun Yoo 
> > > ---
> > 
> > Why is there no other Intel developers willing to review and sign off on
> > this patch?  Please get their review first before asking us to do their
> > work for them :)
> > 
> > thanks,
> > 
> > greg k-h
> > 
> 
> Hi Greg,
> 
> This patch set got our internal review process. Sorry if it's code quality
> is under your expectation but it's the reason why I'm asking you to review
> the code. Could you please share your time to review it?

Nope.  If no other Intel developer thinks it is good enough to put their
name on it as part of their review process, why should I?

Again, please use the resources you have, to fix the obvious problems in
your code, BEFORE asking the community to do that work for you.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/8] [PATCH 1/8] drivers/peci: Add support for PECI bus driver core

[Jae Hyun Yoo]

On 2/21/2018 1:51 PM, Andrew Lunn wrote:

Is there a real need to do transfers in atomic context, or with
interrupts disabled?



Actually, no. Generally, this function will be called in sleep-able context
so this code is for an exceptional case handling.

I'll rewrite this code like below:
if (in_atomic() || irqs_disabled()) {
dev_dbg(>dev,
"xfer in non-sleepable context is not supported\n");
return -EWOULDBLOCK;
}


I would not even do that. Just add a call to
might_sleep(). CONFIG_DEBUG_ATOMIC_SLEEP will then find bad calls.



Thanks for the suggestion. I've learned one thing. :)


+static int peci_ioctl_get_temp(struct peci_adapter *adapter, void *vmsg)
+{
+   struct peci_get_temp_msg *umsg = vmsg;
+   struct peci_xfer_msg msg;
+   int rc;
+


Is this getting the temperature?



Yes, this is getting the 'die' temperature of a processor package.
  
So the hwmon driver provides this. No need to have both.




This this common API in core driver of PECI bus. The hwmon is also uses 
it through peci_command call.



+static long peci_ioctl(struct file *file, unsigned int iocmd, unsigned long 
arg)
+{
+   struct peci_adapter *adapter = file->private_data;
+   void __user *argp = (void __user *)arg;
+   unsigned int msg_len;
+   enum peci_cmd cmd;
+   u8 *msg;
+   int rc = 0;
+
+   dev_dbg(>dev, "ioctl, cmd=0x%x, arg=0x%lx\n", iocmd, arg);
+
+   switch (iocmd) {
+   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:
+   cmd = _IOC_TYPE(iocmd) - PECI_IOC_BASE;
+   msg_len = _IOC_SIZE(iocmd);
+   break;


Adding new ioctl calls is pretty frowned up. Can you export this info
via /sysfs?



Most of these are not simple IOs so ioctl is better suited, I think.


Lets see what other reviewers say, but i think ioctls are
wrong.

  Andrew


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/8] [PATCH 1/8] drivers/peci: Add support for PECI bus driver core

[Andrew Lunn]

> >Is there a real need to do transfers in atomic context, or with
> >interrupts disabled?
> >
> 
> Actually, no. Generally, this function will be called in sleep-able context
> so this code is for an exceptional case handling.
> 
> I'll rewrite this code like below:
>   if (in_atomic() || irqs_disabled()) {
>   dev_dbg(>dev,
>   "xfer in non-sleepable context is not supported\n");
>   return -EWOULDBLOCK;
>   }

I would not even do that. Just add a call to
might_sleep(). CONFIG_DEBUG_ATOMIC_SLEEP will then find bad calls.

> >>+static int peci_ioctl_get_temp(struct peci_adapter *adapter, void *vmsg)
> >>+{
> >>+   struct peci_get_temp_msg *umsg = vmsg;
> >>+   struct peci_xfer_msg msg;
> >>+   int rc;
> >>+
> >
> >Is this getting the temperature?
> >
> 
> Yes, this is getting the 'die' temperature of a processor package.
 
So the hwmon driver provides this. No need to have both.

> >>+static long peci_ioctl(struct file *file, unsigned int iocmd, unsigned 
> >>long arg)
> >>+{
> >>+   struct peci_adapter *adapter = file->private_data;
> >>+   void __user *argp = (void __user *)arg;
> >>+   unsigned int msg_len;
> >>+   enum peci_cmd cmd;
> >>+   u8 *msg;
> >>+   int rc = 0;
> >>+
> >>+   dev_dbg(>dev, "ioctl, cmd=0x%x, arg=0x%lx\n", iocmd, arg);
> >>+
> >>+   switch (iocmd) {
> >>+   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:
> >>+   cmd = _IOC_TYPE(iocmd) - PECI_IOC_BASE;
> >>+   msg_len = _IOC_SIZE(iocmd);
> >>+   break;
> >
> >Adding new ioctl calls is pretty frowned up. Can you export this info
> >via /sysfs?
> >
> 
> Most of these are not simple IOs so ioctl is better suited, I think.

Lets see what other reviewers say, but i think ioctls are
wrong.

 Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/8] [PATCH 1/8] drivers/peci: Add support for PECI bus driver core

[Jae Hyun Yoo]

On 2/21/2018 9:58 AM, Greg KH wrote:

On Wed, Feb 21, 2018 at 08:15:59AM -0800, Jae Hyun Yoo wrote:

This commit adds driver implementation for PECI bus into linux
driver framework.

Signed-off-by: Jae Hyun Yoo 
---


Why is there no other Intel developers willing to review and sign off on
this patch?  Please get their review first before asking us to do their
work for them :)

thanks,

greg k-h



Hi Greg,

This patch set got our internal review process. Sorry if it's code 
quality is under your expectation but it's the reason why I'm asking you 
to review the code. Could you please share your time to review it?


Thanks a lot,
Jae
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/8] [PATCH 1/8] drivers/peci: Add support for PECI bus driver core

[Jae Hyun Yoo]

Hi Andrew,

Thanks for sharing your time to review it. Please check my answers inline.

On 2/21/2018 9:04 AM, Andrew Lunn wrote:

+static int peci_locked_xfer(struct peci_adapter *adapter,
+   struct peci_xfer_msg *msg,
+   bool do_retry,
+   bool has_aw_fcs)
+{
+   ktime_t start, end;
+   s64 elapsed_ms;
+   int rc = 0;
+
+   if (!adapter->xfer) {
+   dev_dbg(>dev, "PECI level transfers not supported\n");
+   return -ENODEV;
+   }
+
+   if (in_atomic() || irqs_disabled()) {


Hi Jae

Is there a real need to do transfers in atomic context, or with
interrupts disabled?



Actually, no. Generally, this function will be called in sleep-able 
context so this code is for an exceptional case handling.


I'll rewrite this code like below:
if (in_atomic() || irqs_disabled()) {
dev_dbg(>dev,
"xfer in non-sleepable context is not supported\n");
return -EWOULDBLOCK;
}

And then, will add a sleep call into the below loop.

I know that in_atomic() call is not recommended in driver code but some 
driver codes still use it since there is no alternative way at this 
time, AFAIK. Please tell me if there is a better solution.



+   rt_mutex_trylock(>bus_lock);
+   if (!rc)
+   return -EAGAIN; /* PECI activity is ongoing */
+   } else {
+   rt_mutex_lock(>bus_lock);
+   }
+
+   if (do_retry)
+   start = ktime_get();
+
+   do {
+   rc = adapter->xfer(adapter, msg);
+
+   if (!do_retry)
+   break;
+
+   /* Per the PECI spec, need to retry commands that return 0x8x */
+   if (!(!rc && ((msg->rx_buf[0] & DEV_PECI_CC_RETRY_ERR_MASK) ==
+ DEV_PECI_CC_TIMEOUT)))
+   break;
+
+   /* Set the retry bit to indicate a retry attempt */
+   msg->tx_buf[1] |= DEV_PECI_RETRY_BIT;
+
+   /* Recalculate the AW FCS if it has one */
+   if (has_aw_fcs)
+   msg->tx_buf[msg->tx_len - 1] = 0x80 ^
+   peci_aw_fcs((u8 *)msg,
+   2 + msg->tx_len);
+
+   /* Retry for at least 250ms before returning an error */
+   end = ktime_get();
+   elapsed_ms = ktime_to_ms(ktime_sub(end, start));
+   if (elapsed_ms >= DEV_PECI_RETRY_TIME_MS) {
+   dev_dbg(>dev, "Timeout retrying xfer!\n");
+   break;
+   }
+   } while (true);


So you busy loop to 1/4 second? How about putting a sleep in here so
other things can be done between each retry.

And should it not return -ETIMEDOUT after that 1/4 second?



Yes, you are right. I'll rewrite this code like below after adding the 
above change:


/**
 * Retry for at least 250ms before returning an error.
 * Retry interval guideline:
 *   No minimum < Retry Interval < No maximum
 *(recommend 10ms)
 */
end = ktime_get();
elapsed_ms = ktime_to_ms(ktime_sub(end, start));
if (elapsed_ms >= DEV_PECI_RETRY_TIME_MS) {
dev_dbg(>dev, "Timeout retrying xfer!\n");
rc = -ETIMEDOUT;
break;
}

usleep_range(DEV_PECI_RETRY_INTERVAL_MS * 1000,
 (DEV_PECI_RETRY_INTERVAL_MS * 1000) + 1000);


+static int peci_scan_cmd_mask(struct peci_adapter *adapter)
+{
+   struct peci_xfer_msg msg;
+   u32 dib;
+   int rc = 0;
+
+   /* Update command mask just once */
+   if (adapter->cmd_mask & BIT(PECI_CMD_PING))
+   return 0;
+
+   msg.addr  = PECI_BASE_ADDR;
+   msg.tx_len= GET_DIB_WR_LEN;
+   msg.rx_len= GET_DIB_RD_LEN;
+   msg.tx_buf[0] = GET_DIB_PECI_CMD;
+
+   rc = peci_xfer(adapter, );
+   if (rc < 0) {
+   dev_dbg(>dev, "PECI xfer error, rc : %d\n", rc);
+   return rc;
+   }
+
+   dib = msg.rx_buf[0] | (msg.rx_buf[1] << 8) |
+ (msg.rx_buf[2] << 16) | (msg.rx_buf[3] << 24);
+
+   /* Check special case for Get DIB command */
+   if (dib == 0x00) {
+   dev_dbg(>dev, "DIB read as 0x00\n");
+   return -1;
+   }
+
+   if (!rc) {
+   /**
+* setting up the supporting commands based on minor rev#
+* see PECI Spec Table 3-1
+*/
+   dib = (dib >> 8) & 0xF;
+
+   if (dib >= 0x1) {
+   adapter->cmd_mask |= BIT(PECI_CMD_RD_PKG_CFG);
+   

Re: [PATCH v2 1/8] [PATCH 1/8] drivers/peci: Add support for PECI bus driver core

[Greg KH]

On Wed, Feb 21, 2018 at 08:15:59AM -0800, Jae Hyun Yoo wrote:
> This commit adds driver implementation for PECI bus into linux
> driver framework.
> 
> Signed-off-by: Jae Hyun Yoo 
> ---

Why is there no other Intel developers willing to review and sign off on
this patch?  Please get their review first before asking us to do their
work for them :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/8] [PATCH 1/8] drivers/peci: Add support for PECI bus driver core

[Andrew Lunn]

> +static int peci_locked_xfer(struct peci_adapter *adapter,
> + struct peci_xfer_msg *msg,
> + bool do_retry,
> + bool has_aw_fcs)
> +{
> + ktime_t start, end;
> + s64 elapsed_ms;
> + int rc = 0;
> +
> + if (!adapter->xfer) {
> + dev_dbg(>dev, "PECI level transfers not supported\n");
> + return -ENODEV;
> + }
> +
> + if (in_atomic() || irqs_disabled()) {

Hi Jae

Is there a real need to do transfers in atomic context, or with
interrupts disabled? 

> + rt_mutex_trylock(>bus_lock);
> + if (!rc)
> + return -EAGAIN; /* PECI activity is ongoing */
> + } else {
> + rt_mutex_lock(>bus_lock);
> + }
> +
> + if (do_retry)
> + start = ktime_get();
> +
> + do {
> + rc = adapter->xfer(adapter, msg);
> +
> + if (!do_retry)
> + break;
> +
> + /* Per the PECI spec, need to retry commands that return 0x8x */
> + if (!(!rc && ((msg->rx_buf[0] & DEV_PECI_CC_RETRY_ERR_MASK) ==
> +   DEV_PECI_CC_TIMEOUT)))
> + break;
> +
> + /* Set the retry bit to indicate a retry attempt */
> + msg->tx_buf[1] |= DEV_PECI_RETRY_BIT;
> +
> + /* Recalculate the AW FCS if it has one */
> + if (has_aw_fcs)
> + msg->tx_buf[msg->tx_len - 1] = 0x80 ^
> + peci_aw_fcs((u8 *)msg,
> + 2 + msg->tx_len);
> +
> + /* Retry for at least 250ms before returning an error */
> + end = ktime_get();
> + elapsed_ms = ktime_to_ms(ktime_sub(end, start));
> + if (elapsed_ms >= DEV_PECI_RETRY_TIME_MS) {
> + dev_dbg(>dev, "Timeout retrying xfer!\n");
> + break;
> + }
> + } while (true);

So you busy loop to 1/4 second? How about putting a sleep in here so
other things can be done between each retry.

And should it not return -ETIMEDOUT after that 1/4 second?

> +static int peci_scan_cmd_mask(struct peci_adapter *adapter)
> +{
> + struct peci_xfer_msg msg;
> + u32 dib;
> + int rc = 0;
> +
> + /* Update command mask just once */
> + if (adapter->cmd_mask & BIT(PECI_CMD_PING))
> + return 0;
> +
> + msg.addr  = PECI_BASE_ADDR;
> + msg.tx_len= GET_DIB_WR_LEN;
> + msg.rx_len= GET_DIB_RD_LEN;
> + msg.tx_buf[0] = GET_DIB_PECI_CMD;
> +
> + rc = peci_xfer(adapter, );
> + if (rc < 0) {
> + dev_dbg(>dev, "PECI xfer error, rc : %d\n", rc);
> + return rc;
> + }
> +
> + dib = msg.rx_buf[0] | (msg.rx_buf[1] << 8) |
> +   (msg.rx_buf[2] << 16) | (msg.rx_buf[3] << 24);
> +
> + /* Check special case for Get DIB command */
> + if (dib == 0x00) {
> + dev_dbg(>dev, "DIB read as 0x00\n");
> + return -1;
> + }
> +
> + if (!rc) {
> + /**
> +  * setting up the supporting commands based on minor rev#
> +  * see PECI Spec Table 3-1
> +  */
> + dib = (dib >> 8) & 0xF;
> +
> + if (dib >= 0x1) {
> + adapter->cmd_mask |= BIT(PECI_CMD_RD_PKG_CFG);
> + adapter->cmd_mask |= BIT(PECI_CMD_WR_PKG_CFG);
> + }
> +
> + if (dib >= 0x2)
> + adapter->cmd_mask |= BIT(PECI_CMD_RD_IA_MSR);
> +
> + if (dib >= 0x3) {
> + adapter->cmd_mask |= BIT(PECI_CMD_RD_PCI_CFG_LOCAL);
> + adapter->cmd_mask |= BIT(PECI_CMD_WR_PCI_CFG_LOCAL);
> + }
> +
> + if (dib >= 0x4)
> + adapter->cmd_mask |= BIT(PECI_CMD_RD_PCI_CFG);
> +
> + if (dib >= 0x5)
> + adapter->cmd_mask |= BIT(PECI_CMD_WR_PCI_CFG);
> +
> + if (dib >= 0x6)
> + adapter->cmd_mask |= BIT(PECI_CMD_WR_IA_MSR);

Lots of magic numbers here. Can they be replaced with #defines.  Also,
it looks like a switch statement could be used, with fall through.

> +
> + adapter->cmd_mask |= BIT(PECI_CMD_GET_TEMP);
> + adapter->cmd_mask |= BIT(PECI_CMD_GET_DIB);
> + adapter->cmd_mask |= BIT(PECI_CMD_PING);
> + } else {
> + dev_dbg(>dev, "Error reading DIB, rc : %d\n", rc);
> + }
> +
> + return rc;
> +}
> +

> +static int peci_ioctl_get_temp(struct peci_adapter *adapter, void *vmsg)
> +{
> + struct peci_get_temp_msg *umsg = vmsg;
> + struct peci_xfer_msg msg;
> + int rc;
> +

Is this getting the temperature?

> + rc = peci_cmd_support(adapter, PECI_CMD_GET_TEMP);
> + if (rc < 0)
> + return rc;
> +
> + msg.addr  = umsg->addr;
> + msg.tx_len= 

[PATCH v2 1/8] [PATCH 1/8] drivers/peci: Add support for PECI bus driver core

[Jae Hyun Yoo]

This commit adds driver implementation for PECI bus into linux
driver framework.

Signed-off-by: Jae Hyun Yoo 
---
 drivers/Kconfig |2 +
 drivers/Makefile|1 +
 drivers/peci/Kconfig|   20 +
 drivers/peci/Makefile   |6 +
 drivers/peci/peci-core.c| 1337 +++
 include/linux/peci.h|   97 +++
 include/uapi/linux/peci-ioctl.h |  207 ++
 7 files changed, 1670 insertions(+)
 create mode 100644 drivers/peci/Kconfig
 create mode 100644 drivers/peci/Makefile
 create mode 100644 drivers/peci/peci-core.c
 create mode 100644 include/linux/peci.h
 create mode 100644 include/uapi/linux/peci-ioctl.h

diff --git a/drivers/Kconfig b/drivers/Kconfig
index 879dc0604cba..031bed5bbe7b 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -219,4 +219,6 @@ source "drivers/siox/Kconfig"
 
 source "drivers/slimbus/Kconfig"
 
+source "drivers/peci/Kconfig"
+
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 24cd47014657..250fe3d0fa7e 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -185,3 +185,4 @@ obj-$(CONFIG_TEE)   += tee/
 obj-$(CONFIG_MULTIPLEXER)  += mux/
 obj-$(CONFIG_UNISYS_VISORBUS)  += visorbus/
 obj-$(CONFIG_SIOX) += siox/
+obj-$(CONFIG_PECI) += peci/
diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig
new file mode 100644
index ..1cd2cb4b2298
--- /dev/null
+++ b/drivers/peci/Kconfig
@@ -0,0 +1,20 @@
+#
+# Platform Environment Control Interface (PECI) subsystem configuration
+#
+
+menu "PECI support"
+
+config PECI
+   tristate "PECI support"
+   select RT_MUTEXES
+   select CRC8
+   help
+ The Platform Environment Control Interface (PECI) is a one-wire bus
+ interface that provides a communication channel between Intel
+ processor and chipset components to external monitoring or control
+ devices.
+
+ This PECI support can also be built as a module.  If so, the module
+ will be called peci-core.
+
+endmenu
diff --git a/drivers/peci/Makefile b/drivers/peci/Makefile
new file mode 100644
index ..9e8615e0d3ff
--- /dev/null
+++ b/drivers/peci/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for the PECI core and bus drivers.
+#
+
+# Core functionality
+obj-$(CONFIG_PECI) += peci-core.o
diff --git a/drivers/peci/peci-core.c b/drivers/peci/peci-core.c
new file mode 100644
index ..d976c7317801
--- /dev/null
+++ b/drivers/peci/peci-core.c
@@ -0,0 +1,1337 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 Intel Corporation
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* Device Specific Completion Code (CC) Definition */
+#define DEV_PECI_CC_RETRY_ERR_MASK  0xf0
+#define DEV_PECI_CC_SUCCESS 0x40
+#define DEV_PECI_CC_TIMEOUT 0x80
+#define DEV_PECI_CC_OUT_OF_RESOURCE 0x81
+#define DEV_PECI_CC_INVALID_REQ 0x90
+
+/* Skylake EDS says to retry for 250ms */
+#define DEV_PECI_RETRY_TIME_MS 250
+#define DEV_PECI_RETRY_BIT 0x01
+
+#define GET_TEMP_WR_LEN   1
+#define GET_TEMP_RD_LEN   2
+#define GET_TEMP_PECI_CMD 0x01
+
+#define GET_DIB_WR_LEN   1
+#define GET_DIB_RD_LEN   8
+#define GET_DIB_PECI_CMD 0xf7
+
+#define RDPKGCFG_WRITE_LEN 5
+#define RDPKGCFG_READ_LEN_BASE 1
+#define RDPKGCFG_PECI_CMD  0xa1
+
+#define WRPKGCFG_WRITE_LEN_BASE 6
+#define WRPKGCFG_READ_LEN   1
+#define WRPKGCFG_PECI_CMD   0xa5
+
+#define RDIAMSR_WRITE_LEN 5
+#define RDIAMSR_READ_LEN  9
+#define RDIAMSR_PECI_CMD  0xb1
+
+#define WRIAMSR_PECI_CMD  0xb5
+
+#define RDPCICFG_WRITE_LEN 6
+#define RDPCICFG_READ_LEN  5
+#define RDPCICFG_PECI_CMD  0x61
+
+#define WRPCICFG_PECI_CMD  0x65
+
+#define RDPCICFGLOCAL_WRITE_LEN 5
+#define RDPCICFGLOCAL_READ_LEN_BASE 1
+#define RDPCICFGLOCAL_PECI_CMD  0xe1
+
+#define WRPCICFGLOCAL_WRITE_LEN_BASE 6
+#define WRPCICFGLOCAL_READ_LEN   1
+#define WRPCICFGLOCAL_PECI_CMD   0xe5
+
+/* CRC8 table for Assure Write Frame Check */
+#define PECI_CRC8_POLYNOMIAL 0x07
+DECLARE_CRC8_TABLE(peci_crc8_table);
+
+static struct device_type peci_adapter_type;
+static struct device_type peci_client_type;
+
+#define PECI_CDEV_MAX 16
+static dev_t peci_devt;
+static bool is_registered;
+
+static DEFINE_MUTEX(core_lock);
+static DEFINE_IDR(peci_adapter_idr);
+
+static ssize_t name_show(struct device *dev,
+struct device_attribute *attr,
+char *buf)
+{
+   return sprintf(buf, "%s\n", dev->type == _client_type ?
+  to_peci_client(dev)->name : to_peci_adapter(dev)->name);
+}
+static DEVICE_ATTR_RO(name);
+
+static void peci_client_dev_release(struct device *dev)
+{
+   kfree(to_peci_client(dev));
+}
+
+static struct attribute *peci_device_attrs[] = {
+   _attr_name.attr,
+   NULL
+};
+ATTRIBUTE_GROUPS(peci_device);
+
+static struct device_type