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-hwmon" 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-hwmon" 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-hwmon" 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-hwmon" 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-hwmon" 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=