[PATCH v2 0/3] platform/x86: intel_scu_ipc: Avoid working around IO and cleanups

[PATCH v2 0/3] platform/x86: intel_scu_ipc: Avoid working around IO and cleanups

[Andy Shevchenko]

The first patch avoids using a workaround for IO (which seems unneeded).
The rest is a batch of cleanups. Has been tested on Intel Merrifield
(thanks, Ferry).

v2:
- dropped stray change (Mika)
- elaborated conditional changes in the commit message (Mika)
- gathered tags (Mika, Ferry)

Andy Shevchenko (3):
  platform/x86: intel_scu_ipc: Replace workaround by 32-bit IO
  platform/x86: intel_scu_ipc: Simplify code with cleanup helpers
  platform/x86: intel_scu_ipc: Save a copy of the entire struct
    intel_scu_ipc_data

 drivers/platform/x86/intel_scu_ipc.c | 141 ++++++++++++---------------
 1 file changed, 60 insertions(+), 81 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 1/3] platform/x86: intel_scu_ipc: Replace workaround by 32-bit IO

[Andy Shevchenko]

The theory is that the so called workaround in pwr_reg_rdwr() is
the actual reader of the data in 32-bit chunks. For some reason
the 8-bit IO won't fail after that. Replace the workaround by using
32-bit IO explicitly and then memcpy() as much data as was requested
by the user. The same approach is already in use in
intel_scu_ipc_dev_command_with_size().

Tested-by: Ferry Toth <fntoth@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel_scu_ipc.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
index 5b16d29c93d7..290b38627542 100644
--- a/drivers/platform/x86/intel_scu_ipc.c
+++ b/drivers/platform/x86/intel_scu_ipc.c
@@ -217,12 +217,6 @@ static inline u8 ipc_read_status(struct intel_scu_ipc_dev *scu)
 	return __raw_readl(scu->ipc_base + IPC_STATUS);
 }
 
-/* Read ipc byte data */
-static inline u8 ipc_data_readb(struct intel_scu_ipc_dev *scu, u32 offset)
-{
-	return readb(scu->ipc_base + IPC_READ_BUFFER + offset);
-}
-
 /* Read ipc u32 data */
 static inline u32 ipc_data_readl(struct intel_scu_ipc_dev *scu, u32 offset)
 {
@@ -325,11 +319,10 @@ static int pwr_reg_rdwr(struct intel_scu_ipc_dev *scu, u16 *addr, u8 *data,
 	}
 
 	err = intel_scu_ipc_check_status(scu);
-	if (!err && id == IPC_CMD_PCNTRL_R) { /* Read rbuf */
-		/* Workaround: values are read as 0 without memcpy_fromio */
-		memcpy_fromio(cbuf, scu->ipc_base + 0x90, 16);
-		for (nc = 0; nc < count; nc++)
-			data[nc] = ipc_data_readb(scu, nc);
+	if (!err) { /* Read rbuf */
+		for (nc = 0, offset = 0; nc < 4; nc++, offset += 4)
+			wbuf[nc] = ipc_data_readl(scu, offset);
+		memcpy(data, wbuf, count);
 	}
 	mutex_unlock(&ipclock);
 	return err;
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v2 1/3] platform/x86: intel_scu_ipc: Replace workaround by 32-bit IO

[Mika Westerberg]

On Mon, Oct 21, 2024 at 11:38:51AM +0300, Andy Shevchenko wrote:
> The theory is that the so called workaround in pwr_reg_rdwr() is
> the actual reader of the data in 32-bit chunks. For some reason
> the 8-bit IO won't fail after that. Replace the workaround by using
> 32-bit IO explicitly and then memcpy() as much data as was requested
> by the user. The same approach is already in use in
> intel_scu_ipc_dev_command_with_size().
> 
> Tested-by: Ferry Toth <fntoth@gmail.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v2 1/3] platform/x86: intel_scu_ipc: Replace workaround by 32-bit IO

[Ilpo Järvinen]

On Mon, 21 Oct 2024, Andy Shevchenko wrote:

> The theory is that the so called workaround in pwr_reg_rdwr() is
> the actual reader of the data in 32-bit chunks. For some reason
> the 8-bit IO won't fail after that. Replace the workaround by using
> 32-bit IO explicitly and then memcpy() as much data as was requested
> by the user. The same approach is already in use in
> intel_scu_ipc_dev_command_with_size().
>
> Tested-by: Ferry Toth <fntoth@gmail.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/platform/x86/intel_scu_ipc.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
> index 5b16d29c93d7..290b38627542 100644
> --- a/drivers/platform/x86/intel_scu_ipc.c
> +++ b/drivers/platform/x86/intel_scu_ipc.c
> @@ -217,12 +217,6 @@ static inline u8 ipc_read_status(struct intel_scu_ipc_dev *scu)
>  	return __raw_readl(scu->ipc_base + IPC_STATUS);
>  }
>  
> -/* Read ipc byte data */
> -static inline u8 ipc_data_readb(struct intel_scu_ipc_dev *scu, u32 offset)
> -{
> -	return readb(scu->ipc_base + IPC_READ_BUFFER + offset);
> -}
> -
>  /* Read ipc u32 data */
>  static inline u32 ipc_data_readl(struct intel_scu_ipc_dev *scu, u32 offset)
>  {
> @@ -325,11 +319,10 @@ static int pwr_reg_rdwr(struct intel_scu_ipc_dev *scu, u16 *addr, u8 *data,
>  	}
>  
>  	err = intel_scu_ipc_check_status(scu);
> -	if (!err && id == IPC_CMD_PCNTRL_R) { /* Read rbuf */
> -		/* Workaround: values are read as 0 without memcpy_fromio */
> -		memcpy_fromio(cbuf, scu->ipc_base + 0x90, 16);
> -		for (nc = 0; nc < count; nc++)
> -			data[nc] = ipc_data_readb(scu, nc);
> +	if (!err) { /* Read rbuf */

What is the reason for the removal of that id check? This seems a clear 
logic change but why? And if you remove want to remove that check, what 
that comment then means?

> +		for (nc = 0, offset = 0; nc < 4; nc++, offset += 4)
> +			wbuf[nc] = ipc_data_readl(scu, offset);
> +		memcpy(data, wbuf, count);

So do we actually need to read more than
DIV_ROUND_UP(min(count, 16U), sizeof(u32))? Because that's the approach 
used in intel_scu_ipc_dev_command_with_size() which you referred to.

>  	}
>  	mutex_unlock(&ipclock);
>  	return err;

FYI (unrelated to this patch), there seems to be some open-coded 
FIELD_PREP()s in pwr_reg_rdwr(), some of which is common code between 
those if branches too.

-- 
 i.

Re: [PATCH v2 1/3] platform/x86: intel_scu_ipc: Replace workaround by 32-bit IO

[Andy Shevchenko]

On Mon, Oct 21, 2024 at 12:24:57PM +0300, Ilpo Järvinen wrote:
> On Mon, 21 Oct 2024, Andy Shevchenko wrote:
> 
> > The theory is that the so called workaround in pwr_reg_rdwr() is
> > the actual reader of the data in 32-bit chunks. For some reason
> > the 8-bit IO won't fail after that. Replace the workaround by using
> > 32-bit IO explicitly and then memcpy() as much data as was requested
> > by the user. The same approach is already in use in
> > intel_scu_ipc_dev_command_with_size().

...

> >  	err = intel_scu_ipc_check_status(scu);
> > -	if (!err && id == IPC_CMD_PCNTRL_R) { /* Read rbuf */
> > -		/* Workaround: values are read as 0 without memcpy_fromio */
> > -		memcpy_fromio(cbuf, scu->ipc_base + 0x90, 16);
> > -		for (nc = 0; nc < count; nc++)
> > -			data[nc] = ipc_data_readb(scu, nc);
> > +	if (!err) { /* Read rbuf */
> 
> What is the reason for the removal of that id check? This seems a clear 
> logic change but why? And if you remove want to remove that check, what 
> that comment then means?

Let me split this to a separate change with better explanation then.

> > +		for (nc = 0, offset = 0; nc < 4; nc++, offset += 4)
> > +			wbuf[nc] = ipc_data_readl(scu, offset);
> > +		memcpy(data, wbuf, count);
> 
> So do we actually need to read more than
> DIV_ROUND_UP(min(count, 16U), sizeof(u32))? Because that's the approach 
> used in intel_scu_ipc_dev_command_with_size() which you referred to.

I'm not sure I follow. We do IO for whole (16-bytes) buffer, but return only
asked _bytes_ to the user.

> >  	}
> >  	mutex_unlock(&ipclock);
> >  	return err;
> 
> FYI (unrelated to this patch), there seems to be some open-coded 
> FIELD_PREP()s in pwr_reg_rdwr(), some of which is common code between 
> those if branches too.

This code is quite old and full of tricks that has to be tested. So, yes
while it's possible to convert, I would like to do it in a small (baby)
steps. This series is already quite intrusive from this perspective :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] platform/x86: intel_scu_ipc: Replace workaround by 32-bit IO

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2341 bytes --]

On Mon, 21 Oct 2024, Andy Shevchenko wrote:

> On Mon, Oct 21, 2024 at 12:24:57PM +0300, Ilpo Järvinen wrote:
> > On Mon, 21 Oct 2024, Andy Shevchenko wrote:
> > 
> > > The theory is that the so called workaround in pwr_reg_rdwr() is
> > > the actual reader of the data in 32-bit chunks. For some reason
> > > the 8-bit IO won't fail after that. Replace the workaround by using
> > > 32-bit IO explicitly and then memcpy() as much data as was requested
> > > by the user. The same approach is already in use in
> > > intel_scu_ipc_dev_command_with_size().
> 
> ...
> 
> > >  	err = intel_scu_ipc_check_status(scu);
> > > -	if (!err && id == IPC_CMD_PCNTRL_R) { /* Read rbuf */
> > > -		/* Workaround: values are read as 0 without memcpy_fromio */
> > > -		memcpy_fromio(cbuf, scu->ipc_base + 0x90, 16);
> > > -		for (nc = 0; nc < count; nc++)
> > > -			data[nc] = ipc_data_readb(scu, nc);
> > > +	if (!err) { /* Read rbuf */
> > 
> > What is the reason for the removal of that id check? This seems a clear 
> > logic change but why? And if you remove want to remove that check, what 
> > that comment then means?
> 
> Let me split this to a separate change with better explanation then.
> 
> > > +		for (nc = 0, offset = 0; nc < 4; nc++, offset += 4)
> > > +			wbuf[nc] = ipc_data_readl(scu, offset);
> > > +		memcpy(data, wbuf, count);
> > 
> > So do we actually need to read more than
> > DIV_ROUND_UP(min(count, 16U), sizeof(u32))? Because that's the approach 
> > used in intel_scu_ipc_dev_command_with_size() which you referred to.
> 
> I'm not sure I follow. We do IO for whole (16-bytes) buffer, but return only
> asked _bytes_ to the user.

So always reading 16 bytes is not part of the old workaround? Because it 
has a "lets read enough" feel.

> > >  	}
> > >  	mutex_unlock(&ipclock);
> > >  	return err;
> > 
> > FYI (unrelated to this patch), there seems to be some open-coded 
> > FIELD_PREP()s in pwr_reg_rdwr(), some of which is common code between 
> > those if branches too.
> 
> This code is quite old and full of tricks that has to be tested. So, yes
> while it's possible to convert, I would like to do it in a small (baby)
> steps. This series is already quite intrusive from this perspective :-)

Yeah, no pressure, I just noted down what I saw. :-)

-- 
 i.

Re: [PATCH v2 1/3] platform/x86: intel_scu_ipc: Replace workaround by 32-bit IO

[Andy Shevchenko]

On Mon, Oct 21, 2024 at 12:49:08PM +0300, Ilpo Järvinen wrote:
> On Mon, 21 Oct 2024, Andy Shevchenko wrote:
> > On Mon, Oct 21, 2024 at 12:24:57PM +0300, Ilpo Järvinen wrote:
> > > On Mon, 21 Oct 2024, Andy Shevchenko wrote:

...

> > > > +		for (nc = 0, offset = 0; nc < 4; nc++, offset += 4)
> > > > +			wbuf[nc] = ipc_data_readl(scu, offset);
> > > > +		memcpy(data, wbuf, count);
> > > 
> > > So do we actually need to read more than
> > > DIV_ROUND_UP(min(count, 16U), sizeof(u32))? Because that's the approach 
> > > used in intel_scu_ipc_dev_command_with_size() which you referred to.
> > 
> > I'm not sure I follow. We do IO for whole (16-bytes) buffer, but return only
> > asked _bytes_ to the user.
> 
> So always reading 16 bytes is not part of the old workaround? Because it 
> has a "lets read enough" feel.

Ah, now I got it! Yes, we may reduce the reads to just needed ones.
The idea is that we always have to perform 32-bit reads independently
on the amount of data we want.

> > > >  	}
> > > >  	mutex_unlock(&ipclock);
> > > >  	return err;
> > > 
> > > FYI (unrelated to this patch), there seems to be some open-coded 
> > > FIELD_PREP()s in pwr_reg_rdwr(), some of which is common code between 
> > > those if branches too.
> > 
> > This code is quite old and full of tricks that has to be tested. So, yes
> > while it's possible to convert, I would like to do it in a small (baby)
> > steps. This series is already quite intrusive from this perspective :-)
> 
> Yeah, no pressure, I just noted down what I saw. :-)

Thanks, I will keep this.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] platform/x86: intel_scu_ipc: Replace workaround by 32-bit IO

[Andy Shevchenko]

On Mon, Oct 21, 2024 at 12:54:16PM +0300, Andy Shevchenko wrote:
> On Mon, Oct 21, 2024 at 12:49:08PM +0300, Ilpo Järvinen wrote:
> > On Mon, 21 Oct 2024, Andy Shevchenko wrote:
> > > On Mon, Oct 21, 2024 at 12:24:57PM +0300, Ilpo Järvinen wrote:
> > > > On Mon, 21 Oct 2024, Andy Shevchenko wrote:

...

> > > > > +		for (nc = 0, offset = 0; nc < 4; nc++, offset += 4)
> > > > > +			wbuf[nc] = ipc_data_readl(scu, offset);
> > > > > +		memcpy(data, wbuf, count);
> > > > 
> > > > So do we actually need to read more than
> > > > DIV_ROUND_UP(min(count, 16U), sizeof(u32))? Because that's the approach 
> > > > used in intel_scu_ipc_dev_command_with_size() which you referred to.
> > > 
> > > I'm not sure I follow. We do IO for whole (16-bytes) buffer, but return only
> > > asked _bytes_ to the user.
> > 
> > So always reading 16 bytes is not part of the old workaround? Because it 
> > has a "lets read enough" feel.
> 
> Ah, now I got it! Yes, we may reduce the reads to just needed ones.
> The idea is that we always have to perform 32-bit reads independently
> on the amount of data we want.

Oh, looking at the code (*) it seems they are really messed up in the original
with bytes vs. 32-bit words! Since the above has been tested, let me put this
on TODO list to clarify this mess and run with another testing.

Sounds good to you?

*) the mythical comment about max 5 items for 20-byte buffer is worrying and
now I know why,

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] platform/x86: intel_scu_ipc: Replace workaround by 32-bit IO

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1761 bytes --]

On Mon, 21 Oct 2024, Andy Shevchenko wrote:

> On Mon, Oct 21, 2024 at 12:54:16PM +0300, Andy Shevchenko wrote:
> > On Mon, Oct 21, 2024 at 12:49:08PM +0300, Ilpo Järvinen wrote:
> > > On Mon, 21 Oct 2024, Andy Shevchenko wrote:
> > > > On Mon, Oct 21, 2024 at 12:24:57PM +0300, Ilpo Järvinen wrote:
> > > > > On Mon, 21 Oct 2024, Andy Shevchenko wrote:
> 
> ...
> 
> > > > > > +		for (nc = 0, offset = 0; nc < 4; nc++, offset += 4)
> > > > > > +			wbuf[nc] = ipc_data_readl(scu, offset);
> > > > > > +		memcpy(data, wbuf, count);
> > > > > 
> > > > > So do we actually need to read more than
> > > > > DIV_ROUND_UP(min(count, 16U), sizeof(u32))? Because that's the approach 
> > > > > used in intel_scu_ipc_dev_command_with_size() which you referred to.
> > > > 
> > > > I'm not sure I follow. We do IO for whole (16-bytes) buffer, but return only
> > > > asked _bytes_ to the user.
> > > 
> > > So always reading 16 bytes is not part of the old workaround? Because it 
> > > has a "lets read enough" feel.
> > 
> > Ah, now I got it! Yes, we may reduce the reads to just needed ones.
> > The idea is that we always have to perform 32-bit reads independently
> > on the amount of data we want.
> 
> Oh, looking at the code (*) it seems they are really messed up in the original
> with bytes vs. 32-bit words! Since the above has been tested, let me put this
> on TODO list to clarify this mess and run with another testing.
> 
> Sounds good to you?

Sure, I'm fine with taking the careful approach.

> *) the mythical comment about max 5 items for 20-byte buffer is worrying and
> now I know why,

Those functions with that comment seem to only be called from 
scu_reg_access() which error checks count > 4.

-- 
 i.

[PATCH v2 2/3] platform/x86: intel_scu_ipc: Simplify code with cleanup helpers

[Andy Shevchenko]

Use macros defined in linux/cleanup.h to automate resource lifetime
control in the driver. The byproduct of this change is that the
negative conditionals are swapped by positive ones that are being
attached to the respective calls, hence the code uses the regular
pattern, i.e. checking for the error first.

Tested-by: Ferry Toth <fntoth@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel_scu_ipc.c | 101 ++++++++++++---------------
 1 file changed, 43 insertions(+), 58 deletions(-)

diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
index 290b38627542..3a1584ed7db8 100644
--- a/drivers/platform/x86/intel_scu_ipc.c
+++ b/drivers/platform/x86/intel_scu_ipc.c
@@ -13,6 +13,7 @@
  * along with other APIs.
  */
 
+#include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/errno.h>
@@ -99,23 +100,21 @@ static struct class intel_scu_ipc_class = {
  */
 struct intel_scu_ipc_dev *intel_scu_ipc_dev_get(void)
 {
-	struct intel_scu_ipc_dev *scu = NULL;
+	guard(mutex)(&ipclock);
 
-	mutex_lock(&ipclock);
 	if (ipcdev) {
 		get_device(&ipcdev->dev);
 		/*
 		 * Prevent the IPC provider from being unloaded while it
 		 * is being used.
 		 */
-		if (!try_module_get(ipcdev->owner))
-			put_device(&ipcdev->dev);
-		else
-			scu = ipcdev;
+		if (try_module_get(ipcdev->owner))
+			return ipcdev;
+
+		put_device(&ipcdev->dev);
 	}
 
-	mutex_unlock(&ipclock);
-	return scu;
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(intel_scu_ipc_dev_get);
 
@@ -289,12 +288,11 @@ static int pwr_reg_rdwr(struct intel_scu_ipc_dev *scu, u16 *addr, u8 *data,
 
 	memset(cbuf, 0, sizeof(cbuf));
 
-	mutex_lock(&ipclock);
+	guard(mutex)(&ipclock);
+
 	scu = intel_scu_ipc_get(scu);
-	if (IS_ERR(scu)) {
-		mutex_unlock(&ipclock);
+	if (IS_ERR(scu))
 		return PTR_ERR(scu);
-	}
 
 	for (nc = 0; nc < count; nc++, offset += 2) {
 		cbuf[offset] = addr[nc];
@@ -319,13 +317,14 @@ static int pwr_reg_rdwr(struct intel_scu_ipc_dev *scu, u16 *addr, u8 *data,
 	}
 
 	err = intel_scu_ipc_check_status(scu);
-	if (!err) { /* Read rbuf */
-		for (nc = 0, offset = 0; nc < 4; nc++, offset += 4)
-			wbuf[nc] = ipc_data_readl(scu, offset);
-		memcpy(data, wbuf, count);
-	}
-	mutex_unlock(&ipclock);
-	return err;
+	if (err)
+		return err;
+
+	for (nc = 0, offset = 0; nc < 4; nc++, offset += 4)
+		wbuf[nc] = ipc_data_readl(scu, offset);
+	memcpy(data, wbuf, count);
+
+	return 0;
 }
 
 /**
@@ -446,17 +445,15 @@ int intel_scu_ipc_dev_simple_command(struct intel_scu_ipc_dev *scu, int cmd,
 	u32 cmdval;
 	int err;
 
-	mutex_lock(&ipclock);
+	guard(mutex)(&ipclock);
+
 	scu = intel_scu_ipc_get(scu);
-	if (IS_ERR(scu)) {
-		mutex_unlock(&ipclock);
+	if (IS_ERR(scu))
 		return PTR_ERR(scu);
-	}
 
 	cmdval = sub << 12 | cmd;
 	ipc_command(scu, cmdval);
 	err = intel_scu_ipc_check_status(scu);
-	mutex_unlock(&ipclock);
 	if (err)
 		dev_err(&scu->dev, "IPC command %#x failed with %d\n", cmdval, err);
 	return err;
@@ -485,18 +482,17 @@ int intel_scu_ipc_dev_command_with_size(struct intel_scu_ipc_dev *scu, int cmd,
 {
 	size_t outbuflen = DIV_ROUND_UP(outlen, sizeof(u32));
 	size_t inbuflen = DIV_ROUND_UP(inlen, sizeof(u32));
-	u32 cmdval, inbuf[4] = {};
+	u32 cmdval, inbuf[4] = {}, outbuf[4] = {};
 	int i, err;
 
 	if (inbuflen > 4 || outbuflen > 4)
 		return -EINVAL;
 
-	mutex_lock(&ipclock);
+	guard(mutex)(&ipclock);
+
 	scu = intel_scu_ipc_get(scu);
-	if (IS_ERR(scu)) {
-		mutex_unlock(&ipclock);
+	if (IS_ERR(scu))
 		return PTR_ERR(scu);
-	}
 
 	memcpy(inbuf, in, inlen);
 	for (i = 0; i < inbuflen; i++)
@@ -505,20 +501,17 @@ int intel_scu_ipc_dev_command_with_size(struct intel_scu_ipc_dev *scu, int cmd,
 	cmdval = (size << 16) | (sub << 12) | cmd;
 	ipc_command(scu, cmdval);
 	err = intel_scu_ipc_check_status(scu);
-
-	if (!err) {
-		u32 outbuf[4] = {};
-
-		for (i = 0; i < outbuflen; i++)
-			outbuf[i] = ipc_data_readl(scu, 4 * i);
-
-		memcpy(out, outbuf, outlen);
+	if (err) {
+		dev_err(&scu->dev, "IPC command %#x failed with %d\n", cmdval, err);
+		return err;
 	}
 
-	mutex_unlock(&ipclock);
-	if (err)
-		dev_err(&scu->dev, "IPC command %#x failed with %d\n", cmdval, err);
-	return err;
+	for (i = 0; i < outbuflen; i++)
+		outbuf[i] = ipc_data_readl(scu, 4 * i);
+
+	memcpy(out, outbuf, outlen);
+
+	return 0;
 }
 EXPORT_SYMBOL(intel_scu_ipc_dev_command_with_size);
 
@@ -572,18 +565,15 @@ __intel_scu_ipc_register(struct device *parent,
 	struct intel_scu_ipc_dev *scu;
 	void __iomem *ipc_base;
 
-	mutex_lock(&ipclock);
+	guard(mutex)(&ipclock);
+
 	/* We support only one IPC */
-	if (ipcdev) {
-		err = -EBUSY;
-		goto err_unlock;
-	}
+	if (ipcdev)
+		return ERR_PTR(-EBUSY);
 
 	scu = kzalloc(sizeof(*scu), GFP_KERNEL);
-	if (!scu) {
-		err = -ENOMEM;
-		goto err_unlock;
-	}
+	if (!scu)
+		return ERR_PTR(-ENOMEM);
 
 	scu->owner = owner;
 	scu->dev.parent = parent;
@@ -621,13 +611,11 @@ __intel_scu_ipc_register(struct device *parent,
 	err = device_register(&scu->dev);
 	if (err) {
 		put_device(&scu->dev);
-		goto err_unlock;
+		return ERR_PTR(err);
 	}
 
 	/* Assign device at last */
 	ipcdev = scu;
-	mutex_unlock(&ipclock);
-
 	return scu;
 
 err_unmap:
@@ -636,9 +624,6 @@ __intel_scu_ipc_register(struct device *parent,
 	release_mem_region(scu_data->mem.start, resource_size(&scu_data->mem));
 err_free:
 	kfree(scu);
-err_unlock:
-	mutex_unlock(&ipclock);
-
 	return ERR_PTR(err);
 }
 EXPORT_SYMBOL_GPL(__intel_scu_ipc_register);
@@ -652,12 +637,12 @@ EXPORT_SYMBOL_GPL(__intel_scu_ipc_register);
  */
 void intel_scu_ipc_unregister(struct intel_scu_ipc_dev *scu)
 {
-	mutex_lock(&ipclock);
+	guard(mutex)(&ipclock);
+
 	if (!WARN_ON(!ipcdev)) {
 		ipcdev = NULL;
 		device_unregister(&scu->dev);
 	}
-	mutex_unlock(&ipclock);
 }
 EXPORT_SYMBOL_GPL(intel_scu_ipc_unregister);
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v2 2/3] platform/x86: intel_scu_ipc: Simplify code with cleanup helpers

[Mika Westerberg]

On Mon, Oct 21, 2024 at 11:38:52AM +0300, Andy Shevchenko wrote:
> Use macros defined in linux/cleanup.h to automate resource lifetime
> control in the driver. The byproduct of this change is that the
> negative conditionals are swapped by positive ones that are being
> attached to the respective calls, hence the code uses the regular
> pattern, i.e. checking for the error first.
> 
> Tested-by: Ferry Toth <fntoth@gmail.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v2 2/3] platform/x86: intel_scu_ipc: Simplify code with cleanup helpers

[Ilpo Järvinen]

On Mon, 21 Oct 2024, Andy Shevchenko wrote:

> Use macros defined in linux/cleanup.h to automate resource lifetime
> control in the driver. The byproduct of this change is that the
> negative conditionals are swapped by positive ones that are being
> attached to the respective calls, hence the code uses the regular
> pattern, i.e. checking for the error first.
> 
> Tested-by: Ferry Toth <fntoth@gmail.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/platform/x86/intel_scu_ipc.c | 101 ++++++++++++---------------
>  1 file changed, 43 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
> index 290b38627542..3a1584ed7db8 100644
> --- a/drivers/platform/x86/intel_scu_ipc.c
> +++ b/drivers/platform/x86/intel_scu_ipc.c
> @@ -13,6 +13,7 @@
>   * along with other APIs.
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/errno.h>
> @@ -99,23 +100,21 @@ static struct class intel_scu_ipc_class = {
>   */
>  struct intel_scu_ipc_dev *intel_scu_ipc_dev_get(void)
>  {
> -	struct intel_scu_ipc_dev *scu = NULL;
> +	guard(mutex)(&ipclock);
>  
> -	mutex_lock(&ipclock);
>  	if (ipcdev) {
>  		get_device(&ipcdev->dev);
>  		/*
>  		 * Prevent the IPC provider from being unloaded while it
>  		 * is being used.
>  		 */
> -		if (!try_module_get(ipcdev->owner))
> -			put_device(&ipcdev->dev);
> -		else
> -			scu = ipcdev;
> +		if (try_module_get(ipcdev->owner))
> +			return ipcdev;
> +
> +		put_device(&ipcdev->dev);
>  	}
>  
> -	mutex_unlock(&ipclock);
> -	return scu;
> +	return NULL;
>  }
>  EXPORT_SYMBOL_GPL(intel_scu_ipc_dev_get);
>  
> @@ -289,12 +288,11 @@ static int pwr_reg_rdwr(struct intel_scu_ipc_dev *scu, u16 *addr, u8 *data,
>  
>  	memset(cbuf, 0, sizeof(cbuf));
>  
> -	mutex_lock(&ipclock);
> +	guard(mutex)(&ipclock);
> +
>  	scu = intel_scu_ipc_get(scu);
> -	if (IS_ERR(scu)) {
> -		mutex_unlock(&ipclock);
> +	if (IS_ERR(scu))
>  		return PTR_ERR(scu);
> -	}
>  
>  	for (nc = 0; nc < count; nc++, offset += 2) {
>  		cbuf[offset] = addr[nc];
> @@ -319,13 +317,14 @@ static int pwr_reg_rdwr(struct intel_scu_ipc_dev *scu, u16 *addr, u8 *data,
>  	}
>  
>  	err = intel_scu_ipc_check_status(scu);
> -	if (!err) { /* Read rbuf */
> -		for (nc = 0, offset = 0; nc < 4; nc++, offset += 4)
> -			wbuf[nc] = ipc_data_readl(scu, offset);
> -		memcpy(data, wbuf, count);
> -	}
> -	mutex_unlock(&ipclock);
> -	return err;
> +	if (err)
> +		return err;
> +
> +	for (nc = 0, offset = 0; nc < 4; nc++, offset += 4)
> +		wbuf[nc] = ipc_data_readl(scu, offset);
> +	memcpy(data, wbuf, count);
> +
> +	return 0;
>  }
>  
>  /**
> @@ -446,17 +445,15 @@ int intel_scu_ipc_dev_simple_command(struct intel_scu_ipc_dev *scu, int cmd,
>  	u32 cmdval;
>  	int err;
>  
> -	mutex_lock(&ipclock);
> +	guard(mutex)(&ipclock);
> +
>  	scu = intel_scu_ipc_get(scu);
> -	if (IS_ERR(scu)) {
> -		mutex_unlock(&ipclock);
> +	if (IS_ERR(scu))
>  		return PTR_ERR(scu);
> -	}
>  
>  	cmdval = sub << 12 | cmd;
>  	ipc_command(scu, cmdval);
>  	err = intel_scu_ipc_check_status(scu);
> -	mutex_unlock(&ipclock);
>  	if (err)
>  		dev_err(&scu->dev, "IPC command %#x failed with %d\n", cmdval, err);
>  	return err;
> @@ -485,18 +482,17 @@ int intel_scu_ipc_dev_command_with_size(struct intel_scu_ipc_dev *scu, int cmd,
>  {
>  	size_t outbuflen = DIV_ROUND_UP(outlen, sizeof(u32));
>  	size_t inbuflen = DIV_ROUND_UP(inlen, sizeof(u32));
> -	u32 cmdval, inbuf[4] = {};
> +	u32 cmdval, inbuf[4] = {}, outbuf[4] = {};
>  	int i, err;
>  
>  	if (inbuflen > 4 || outbuflen > 4)
>  		return -EINVAL;
>  
> -	mutex_lock(&ipclock);
> +	guard(mutex)(&ipclock);
> +
>  	scu = intel_scu_ipc_get(scu);
> -	if (IS_ERR(scu)) {
> -		mutex_unlock(&ipclock);
> +	if (IS_ERR(scu))
>  		return PTR_ERR(scu);
> -	}
>  
>  	memcpy(inbuf, in, inlen);
>  	for (i = 0; i < inbuflen; i++)
> @@ -505,20 +501,17 @@ int intel_scu_ipc_dev_command_with_size(struct intel_scu_ipc_dev *scu, int cmd,
>  	cmdval = (size << 16) | (sub << 12) | cmd;
>  	ipc_command(scu, cmdval);
>  	err = intel_scu_ipc_check_status(scu);
> -
> -	if (!err) {
> -		u32 outbuf[4] = {};
> -
> -		for (i = 0; i < outbuflen; i++)
> -			outbuf[i] = ipc_data_readl(scu, 4 * i);
> -
> -		memcpy(out, outbuf, outlen);
> +	if (err) {
> +		dev_err(&scu->dev, "IPC command %#x failed with %d\n", cmdval, err);
> +		return err;
>  	}
>  
> -	mutex_unlock(&ipclock);
> -	if (err)
> -		dev_err(&scu->dev, "IPC command %#x failed with %d\n", cmdval, err);
> -	return err;
> +	for (i = 0; i < outbuflen; i++)
> +		outbuf[i] = ipc_data_readl(scu, 4 * i);
> +
> +	memcpy(out, outbuf, outlen);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(intel_scu_ipc_dev_command_with_size);
>  
> @@ -572,18 +565,15 @@ __intel_scu_ipc_register(struct device *parent,
>  	struct intel_scu_ipc_dev *scu;
>  	void __iomem *ipc_base;
>  
> -	mutex_lock(&ipclock);
> +	guard(mutex)(&ipclock);
> +
>  	/* We support only one IPC */
> -	if (ipcdev) {
> -		err = -EBUSY;
> -		goto err_unlock;
> -	}
> +	if (ipcdev)
> +		return ERR_PTR(-EBUSY);
>  
>  	scu = kzalloc(sizeof(*scu), GFP_KERNEL);
> -	if (!scu) {
> -		err = -ENOMEM;
> -		goto err_unlock;
> -	}
> +	if (!scu)
> +		return ERR_PTR(-ENOMEM);
>  
>  	scu->owner = owner;
>  	scu->dev.parent = parent;
> @@ -621,13 +611,11 @@ __intel_scu_ipc_register(struct device *parent,
>  	err = device_register(&scu->dev);
>  	if (err) {
>  		put_device(&scu->dev);
> -		goto err_unlock;
> +		return ERR_PTR(err);
>  	}
>  
>  	/* Assign device at last */
>  	ipcdev = scu;
> -	mutex_unlock(&ipclock);
> -
>  	return scu;
>  
>  err_unmap:
> @@ -636,9 +624,6 @@ __intel_scu_ipc_register(struct device *parent,
>  	release_mem_region(scu_data->mem.start, resource_size(&scu_data->mem));
>  err_free:
>  	kfree(scu);
> -err_unlock:
> -	mutex_unlock(&ipclock);
> -
>  	return ERR_PTR(err);
>  }
>  EXPORT_SYMBOL_GPL(__intel_scu_ipc_register);
> @@ -652,12 +637,12 @@ EXPORT_SYMBOL_GPL(__intel_scu_ipc_register);
>   */
>  void intel_scu_ipc_unregister(struct intel_scu_ipc_dev *scu)
>  {
> -	mutex_lock(&ipclock);
> +	guard(mutex)(&ipclock);
> +
>  	if (!WARN_ON(!ipcdev)) {
>  		ipcdev = NULL;
>  		device_unregister(&scu->dev);
>  	}
> -	mutex_unlock(&ipclock);
>  }
>  EXPORT_SYMBOL_GPL(intel_scu_ipc_unregister);

IMO, this change is doing too many things at once and it's hard to justify 
why those changes must be kept in the same patch. If the guard() change 
is done first and only then the logic reversions, both patches would 
probably be near trivial to review for correctness.

-- 
 i.

Re: [PATCH v2 2/3] platform/x86: intel_scu_ipc: Simplify code with cleanup helpers

[Andy Shevchenko]

On Mon, Oct 21, 2024 at 12:32 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
> On Mon, 21 Oct 2024, Andy Shevchenko wrote:

...

> IMO, this change is doing too many things at once and it's hard to justify
> why those changes must be kept in the same patch. If the guard() change
> is done first and only then the logic reversions, both patches would
> probably be near trivial to review for correctness.

Are you insisting on this?
Because that's how I have done similar changes in the past all over
the kernel, and IIRC you are the first one asking for this :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] platform/x86: intel_scu_ipc: Simplify code with cleanup helpers

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]

On Mon, 21 Oct 2024, Andy Shevchenko wrote:

> On Mon, Oct 21, 2024 at 12:32 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> > On Mon, 21 Oct 2024, Andy Shevchenko wrote:
> 
> ...
> 
> > IMO, this change is doing too many things at once and it's hard to justify
> > why those changes must be kept in the same patch. If the guard() change
> > is done first and only then the logic reversions, both patches would
> > probably be near trivial to review for correctness.
> 
> Are you insisting on this?
> Because that's how I have done similar changes in the past all over
> the kernel, and IIRC you are the first one asking for this :-)

Well, I know I could go through the patch as is and likely find out it's 
correct. But as is, it requires clearly more effort that it would if those 
two things would be separated. The contexts would be much smaller and 
focused if you split this into two and since you know the end result (the 
current patch), the second patch is just the diff of the first to it.

I'm not saying it's always required but TBH, this patch definitely would 
get simpler to read if you split it into two. So to answer your question, 
it's more of a judgement call than insisting it always.


-- 
 i.

[PATCH v2 3/3] platform/x86: intel_scu_ipc: Save a copy of the entire struct intel_scu_ipc_data

[Andy Shevchenko]

Save a copy of the entire struct intel_scu_ipc_data for easier
maintenance in case of expanding (adding new members become simpler).

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Tested-by: Ferry Toth <fntoth@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel_scu_ipc.c | 33 ++++++++++++++--------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
index 3a1584ed7db8..f7db90a14afd 100644
--- a/drivers/platform/x86/intel_scu_ipc.c
+++ b/drivers/platform/x86/intel_scu_ipc.c
@@ -57,11 +57,11 @@
 
 struct intel_scu_ipc_dev {
 	struct device dev;
-	struct resource mem;
 	struct module *owner;
-	int irq;
 	void __iomem *ipc_base;
 	struct completion cmd_complete;
+
+	struct intel_scu_ipc_data data;
 };
 
 #define IPC_STATUS		0x04
@@ -255,7 +255,7 @@ static inline int ipc_wait_for_interrupt(struct intel_scu_ipc_dev *scu)
 
 static int intel_scu_ipc_check_status(struct intel_scu_ipc_dev *scu)
 {
-	return scu->irq > 0 ? ipc_wait_for_interrupt(scu) : busy_loop(scu);
+	return scu->data.irq > 0 ? ipc_wait_for_interrupt(scu) : busy_loop(scu);
 }
 
 static struct intel_scu_ipc_dev *intel_scu_ipc_get(struct intel_scu_ipc_dev *scu)
@@ -535,13 +535,13 @@ static irqreturn_t ioc(int irq, void *dev_id)
 
 static void intel_scu_ipc_release(struct device *dev)
 {
-	struct intel_scu_ipc_dev *scu;
+	struct intel_scu_ipc_dev *scu = container_of(dev, struct intel_scu_ipc_dev, dev);
+	struct intel_scu_ipc_data *data = &scu->data;
 
-	scu = container_of(dev, struct intel_scu_ipc_dev, dev);
-	if (scu->irq > 0)
-		free_irq(scu->irq, scu);
+	if (data->irq > 0)
+		free_irq(data->irq, scu);
 	iounmap(scu->ipc_base);
-	release_mem_region(scu->mem.start, resource_size(&scu->mem));
+	release_mem_region(data->mem.start, resource_size(&data->mem));
 	kfree(scu);
 }
 
@@ -562,6 +562,7 @@ __intel_scu_ipc_register(struct device *parent,
 			 struct module *owner)
 {
 	int err;
+	struct intel_scu_ipc_data *data;
 	struct intel_scu_ipc_dev *scu;
 	void __iomem *ipc_base;
 
@@ -580,25 +581,25 @@ __intel_scu_ipc_register(struct device *parent,
 	scu->dev.class = &intel_scu_ipc_class;
 	scu->dev.release = intel_scu_ipc_release;
 
-	if (!request_mem_region(scu_data->mem.start, resource_size(&scu_data->mem),
-				"intel_scu_ipc")) {
+	memcpy(&scu->data, scu_data, sizeof(scu->data));
+	data = &scu->data;
+
+	if (!request_mem_region(data->mem.start, resource_size(&data->mem), "intel_scu_ipc")) {
 		err = -EBUSY;
 		goto err_free;
 	}
 
-	ipc_base = ioremap(scu_data->mem.start, resource_size(&scu_data->mem));
+	ipc_base = ioremap(data->mem.start, resource_size(&data->mem));
 	if (!ipc_base) {
 		err = -ENOMEM;
 		goto err_release;
 	}
 
 	scu->ipc_base = ipc_base;
-	scu->mem = scu_data->mem;
-	scu->irq = scu_data->irq;
 	init_completion(&scu->cmd_complete);
 
-	if (scu->irq > 0) {
-		err = request_irq(scu->irq, ioc, 0, "intel_scu_ipc", scu);
+	if (data->irq > 0) {
+		err = request_irq(data->irq, ioc, 0, "intel_scu_ipc", scu);
 		if (err)
 			goto err_unmap;
 	}
@@ -621,7 +622,7 @@ __intel_scu_ipc_register(struct device *parent,
 err_unmap:
 	iounmap(ipc_base);
 err_release:
-	release_mem_region(scu_data->mem.start, resource_size(&scu_data->mem));
+	release_mem_region(data->mem.start, resource_size(&data->mem));
 err_free:
 	kfree(scu);
 	return ERR_PTR(err);
-- 
2.43.0.rc1.1336.g36b5255a03ac