[PATCH v1 0/3] soc: mediatek: mtk-cmdq-helper: Various cleanups

[PATCH v1 0/3] soc: mediatek: mtk-cmdq-helper: Various cleanups

[AngeloGioacchino Del Regno]

This series performs various cleanups to the MediaTek CMDQ Helper lib,
reducing code duplication and enhancing human readability.

This also avoids double initialization struct cmdq_instruction as,
in some cases, it was stack-initialized to zero and then overwritten
completely anyway a bit later.
I'd expect compilers to be somehow smart about that, but still, while
at it ... why not :-)

Tested on MT8192 Asurada, MT8195 Tomato Chromebooks.

AngeloGioacchino Del Regno (3):
  soc: mediatek: mtk-cmdq: Move mask build and append to function
  soc: mediatek: mtk-cmdq: Mark very unlikely branches as such
  soc: mediatek: mtk-cmdq: Move cmdq_instruction init to declaration

 drivers/soc/mediatek/mtk-cmdq-helper.c | 241 ++++++++++++-------------
 1 file changed, 112 insertions(+), 129 deletions(-)

-- 
2.46.0

[PATCH v1 1/3] soc: mediatek: mtk-cmdq: Move mask build and append to function

[AngeloGioacchino Del Regno]

Move the CMDQ_CODE_MASK packet build and append logic to a new
cmdq_pkt_mask() function; this reduces code duplication by 4x.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/soc/mediatek/mtk-cmdq-helper.c | 31 ++++++++++++--------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index a8fccedba83f..620c371fd1fc 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -180,6 +180,15 @@ static int cmdq_pkt_append_command(struct cmdq_pkt *pkt,
 	return 0;
 }
 
+static int cmdq_pkt_mask(struct cmdq_pkt *pkt, u32 mask)
+{
+	struct cmdq_instruction inst = {
+		.op = CMDQ_CODE_MASK,
+		.mask = ~mask
+	};
+	return cmdq_pkt_append_command(pkt, inst);
+}
+
 int cmdq_pkt_write(struct cmdq_pkt *pkt, u8 subsys, u16 offset, u32 value)
 {
 	struct cmdq_instruction inst;
@@ -196,14 +205,11 @@ EXPORT_SYMBOL(cmdq_pkt_write);
 int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys,
 			u16 offset, u32 value, u32 mask)
 {
-	struct cmdq_instruction inst = { {0} };
 	u16 offset_mask = offset;
 	int err;
 
 	if (mask != 0xffffffff) {
-		inst.op = CMDQ_CODE_MASK;
-		inst.mask = ~mask;
-		err = cmdq_pkt_append_command(pkt, inst);
+		err = cmdq_pkt_mask(pkt, mask);
 		if (err < 0)
 			return err;
 
@@ -251,9 +257,7 @@ int cmdq_pkt_write_s_mask(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
 	struct cmdq_instruction inst = {};
 	int err;
 
-	inst.op = CMDQ_CODE_MASK;
-	inst.mask = ~mask;
-	err = cmdq_pkt_append_command(pkt, inst);
+	err = cmdq_pkt_mask(pkt, mask);
 	if (err < 0)
 		return err;
 
@@ -288,9 +292,7 @@ int cmdq_pkt_write_s_mask_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
 	struct cmdq_instruction inst = {};
 	int err;
 
-	inst.op = CMDQ_CODE_MASK;
-	inst.mask = ~mask;
-	err = cmdq_pkt_append_command(pkt, inst);
+	err = cmdq_pkt_mask(pkt, mask);
 	if (err < 0)
 		return err;
 
@@ -409,12 +411,9 @@ EXPORT_SYMBOL(cmdq_pkt_poll);
 int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
 		       u16 offset, u32 value, u32 mask)
 {
-	struct cmdq_instruction inst = { {0} };
 	int err;
 
-	inst.op = CMDQ_CODE_MASK;
-	inst.mask = ~mask;
-	err = cmdq_pkt_append_command(pkt, inst);
+	err = cmdq_pkt_mask(pkt, mask);
 	if (err < 0)
 		return err;
 
@@ -436,9 +435,7 @@ int cmdq_pkt_poll_addr(struct cmdq_pkt *pkt, dma_addr_t addr, u32 value, u32 mas
 	 * which enables use_mask bit.
 	 */
 	if (mask != GENMASK(31, 0)) {
-		inst.op = CMDQ_CODE_MASK;
-		inst.mask = ~mask;
-		ret = cmdq_pkt_append_command(pkt, inst);
+		ret = cmdq_pkt_mask(pkt, mask);
 		if (ret < 0)
 			return ret;
 		use_mask = CMDQ_POLL_ENABLE_MASK;
-- 
2.46.0

Re: [PATCH v1 1/3] soc: mediatek: mtk-cmdq: Move mask build and append to function

[Matthias Brugger]

On 18/09/2024 12:06, AngeloGioacchino Del Regno wrote:
> Move the CMDQ_CODE_MASK packet build and append logic to a new
> cmdq_pkt_mask() function; this reduces code duplication by 4x.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

> ---
>   drivers/soc/mediatek/mtk-cmdq-helper.c | 31 ++++++++++++--------------
>   1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index a8fccedba83f..620c371fd1fc 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -180,6 +180,15 @@ static int cmdq_pkt_append_command(struct cmdq_pkt *pkt,
>   	return 0;
>   }
>   
> +static int cmdq_pkt_mask(struct cmdq_pkt *pkt, u32 mask)
> +{
> +	struct cmdq_instruction inst = {
> +		.op = CMDQ_CODE_MASK,
> +		.mask = ~mask
> +	};
> +	return cmdq_pkt_append_command(pkt, inst);
> +}
> +
>   int cmdq_pkt_write(struct cmdq_pkt *pkt, u8 subsys, u16 offset, u32 value)
>   {
>   	struct cmdq_instruction inst;
> @@ -196,14 +205,11 @@ EXPORT_SYMBOL(cmdq_pkt_write);
>   int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys,
>   			u16 offset, u32 value, u32 mask)
>   {
> -	struct cmdq_instruction inst = { {0} };
>   	u16 offset_mask = offset;
>   	int err;
>   
>   	if (mask != 0xffffffff) {
> -		inst.op = CMDQ_CODE_MASK;
> -		inst.mask = ~mask;
> -		err = cmdq_pkt_append_command(pkt, inst);
> +		err = cmdq_pkt_mask(pkt, mask);
>   		if (err < 0)
>   			return err;
>   
> @@ -251,9 +257,7 @@ int cmdq_pkt_write_s_mask(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>   	struct cmdq_instruction inst = {};
>   	int err;
>   
> -	inst.op = CMDQ_CODE_MASK;
> -	inst.mask = ~mask;
> -	err = cmdq_pkt_append_command(pkt, inst);
> +	err = cmdq_pkt_mask(pkt, mask);
>   	if (err < 0)
>   		return err;
>   
> @@ -288,9 +292,7 @@ int cmdq_pkt_write_s_mask_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
>   	struct cmdq_instruction inst = {};
>   	int err;
>   
> -	inst.op = CMDQ_CODE_MASK;
> -	inst.mask = ~mask;
> -	err = cmdq_pkt_append_command(pkt, inst);
> +	err = cmdq_pkt_mask(pkt, mask);
>   	if (err < 0)
>   		return err;
>   
> @@ -409,12 +411,9 @@ EXPORT_SYMBOL(cmdq_pkt_poll);
>   int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
>   		       u16 offset, u32 value, u32 mask)
>   {
> -	struct cmdq_instruction inst = { {0} };
>   	int err;
>   
> -	inst.op = CMDQ_CODE_MASK;
> -	inst.mask = ~mask;
> -	err = cmdq_pkt_append_command(pkt, inst);
> +	err = cmdq_pkt_mask(pkt, mask);
>   	if (err < 0)
>   		return err;
>   
> @@ -436,9 +435,7 @@ int cmdq_pkt_poll_addr(struct cmdq_pkt *pkt, dma_addr_t addr, u32 value, u32 mas
>   	 * which enables use_mask bit.
>   	 */
>   	if (mask != GENMASK(31, 0)) {
> -		inst.op = CMDQ_CODE_MASK;
> -		inst.mask = ~mask;
> -		ret = cmdq_pkt_append_command(pkt, inst);
> +		ret = cmdq_pkt_mask(pkt, mask);
>   		if (ret < 0)
>   			return ret;
>   		use_mask = CMDQ_POLL_ENABLE_MASK;

[PATCH v1 2/3] soc: mediatek: mtk-cmdq: Mark very unlikely branches as such

[AngeloGioacchino Del Regno]

Calling cmdq packet builders with an unsupported event number,
or without left/right operands (in the case of logic commands)
means that the caller (another driver) wants to perform an
unsupported operation, so this means that the caller must be
fixed instead.

Anyway, such checks are here for safety and, unless any driver
bug or any kind of misconfiguration is present, will always be
false so add a very unlikely hint for those.

Knowing that CPUs' branch predictors (and compilers, anyway) are
indeed smart about these cases, this is done mainly for human
readability purposes.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/soc/mediatek/mtk-cmdq-helper.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index 620c371fd1fc..4ffd1a35df87 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -336,7 +336,7 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear)
 	struct cmdq_instruction inst = { {0} };
 	u32 clear_option = clear ? CMDQ_WFE_UPDATE : 0;
 
-	if (event >= CMDQ_MAX_EVENT)
+	if (unlikely(event >= CMDQ_MAX_EVENT))
 		return -EINVAL;
 
 	inst.op = CMDQ_CODE_WFE;
@@ -351,7 +351,7 @@ int cmdq_pkt_acquire_event(struct cmdq_pkt *pkt, u16 event)
 {
 	struct cmdq_instruction inst = {};
 
-	if (event >= CMDQ_MAX_EVENT)
+	if (unlikely(event >= CMDQ_MAX_EVENT))
 		return -EINVAL;
 
 	inst.op = CMDQ_CODE_WFE;
@@ -366,7 +366,7 @@ int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
 {
 	struct cmdq_instruction inst = { {0} };
 
-	if (event >= CMDQ_MAX_EVENT)
+	if (unlikely(event >= CMDQ_MAX_EVENT))
 		return -EINVAL;
 
 	inst.op = CMDQ_CODE_WFE;
@@ -381,7 +381,7 @@ int cmdq_pkt_set_event(struct cmdq_pkt *pkt, u16 event)
 {
 	struct cmdq_instruction inst = {};
 
-	if (event >= CMDQ_MAX_EVENT)
+	if (unlikely(event >= CMDQ_MAX_EVENT))
 		return -EINVAL;
 
 	inst.op = CMDQ_CODE_WFE;
@@ -476,7 +476,7 @@ int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, u16 result_reg_idx,
 {
 	struct cmdq_instruction inst = { {0} };
 
-	if (!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX)
+	if (unlikely(!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX))
 		return -EINVAL;
 
 	inst.op = CMDQ_CODE_LOGIC;
-- 
2.46.0

Re: [PATCH v1 2/3] soc: mediatek: mtk-cmdq: Mark very unlikely branches as such

[Matthias Brugger]

On 18/09/2024 12:06, AngeloGioacchino Del Regno wrote:
> Calling cmdq packet builders with an unsupported event number,
> or without left/right operands (in the case of logic commands)
> means that the caller (another driver) wants to perform an
> unsupported operation, so this means that the caller must be
> fixed instead.
> 
> Anyway, such checks are here for safety and, unless any driver
> bug or any kind of misconfiguration is present, will always be
> false so add a very unlikely hint for those.
> 
> Knowing that CPUs' branch predictors (and compilers, anyway) are
> indeed smart about these cases, this is done mainly for human
> readability purposes.
> 

Are you really sure we need that? As you mentioned the unlikely() is probably 
useless as compiler and branch predictions will do the job. I don't see the 
complexity in the code to have this annotations for human readability.

I would argue against using unlikely() here as, in general, it is discouraged to 
use it. We will just create a data point of doing things that should only be 
done with very good reason. I don't see the reason here, it will only confuse 
other developers about the use of likely() and unlikely().

Regards,
Matthias

> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/soc/mediatek/mtk-cmdq-helper.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index 620c371fd1fc..4ffd1a35df87 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -336,7 +336,7 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear)
>   	struct cmdq_instruction inst = { {0} };
>   	u32 clear_option = clear ? CMDQ_WFE_UPDATE : 0;
>   
> -	if (event >= CMDQ_MAX_EVENT)
> +	if (unlikely(event >= CMDQ_MAX_EVENT))
>   		return -EINVAL;
>   
>   	inst.op = CMDQ_CODE_WFE;
> @@ -351,7 +351,7 @@ int cmdq_pkt_acquire_event(struct cmdq_pkt *pkt, u16 event)
>   {
>   	struct cmdq_instruction inst = {};
>   
> -	if (event >= CMDQ_MAX_EVENT)
> +	if (unlikely(event >= CMDQ_MAX_EVENT))
>   		return -EINVAL;
>   
>   	inst.op = CMDQ_CODE_WFE;
> @@ -366,7 +366,7 @@ int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
>   {
>   	struct cmdq_instruction inst = { {0} };
>   
> -	if (event >= CMDQ_MAX_EVENT)
> +	if (unlikely(event >= CMDQ_MAX_EVENT))
>   		return -EINVAL;
>   
>   	inst.op = CMDQ_CODE_WFE;
> @@ -381,7 +381,7 @@ int cmdq_pkt_set_event(struct cmdq_pkt *pkt, u16 event)
>   {
>   	struct cmdq_instruction inst = {};
>   
> -	if (event >= CMDQ_MAX_EVENT)
> +	if (unlikely(event >= CMDQ_MAX_EVENT))
>   		return -EINVAL;
>   
>   	inst.op = CMDQ_CODE_WFE;
> @@ -476,7 +476,7 @@ int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, u16 result_reg_idx,
>   {
>   	struct cmdq_instruction inst = { {0} };
>   
> -	if (!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX)
> +	if (unlikely(!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX))
>   		return -EINVAL;
>   
>   	inst.op = CMDQ_CODE_LOGIC;

Re: [PATCH v1 2/3] soc: mediatek: mtk-cmdq: Mark very unlikely branches as such

[AngeloGioacchino Del Regno]

Il 02/10/24 14:41, Matthias Brugger ha scritto:
> 
> 
> On 18/09/2024 12:06, AngeloGioacchino Del Regno wrote:
>> Calling cmdq packet builders with an unsupported event number,
>> or without left/right operands (in the case of logic commands)
>> means that the caller (another driver) wants to perform an
>> unsupported operation, so this means that the caller must be
>> fixed instead.
>>
>> Anyway, such checks are here for safety and, unless any driver
>> bug or any kind of misconfiguration is present, will always be
>> false so add a very unlikely hint for those.
>>
>> Knowing that CPUs' branch predictors (and compilers, anyway) are
>> indeed smart about these cases, this is done mainly for human
>> readability purposes.
>>
> 
> Are you really sure we need that? As you mentioned the unlikely() is probably 
> useless as compiler and branch predictions will do the job. I don't see the 
> complexity in the code to have this annotations for human readability.
> 
> I would argue against using unlikely() here as, in general, it is discouraged to 
> use it. We will just create a data point of doing things that should only be done 
> with very good reason. I don't see the reason here, it will only confuse other 
> developers about the use of likely() and unlikely().
> 

If you have strong opinions I have no problem dropping this.
Perhaps I can add a comment stating "this is very unlikely to happen and should
be dropped after thorough cleanup", if that's better?

Cheers!
Angelo

> Regards,
> Matthias
> 
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/soc/mediatek/mtk-cmdq-helper.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c 
>> b/drivers/soc/mediatek/mtk-cmdq-helper.c
>> index 620c371fd1fc..4ffd1a35df87 100644
>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
>> @@ -336,7 +336,7 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear)
>>       struct cmdq_instruction inst = { {0} };
>>       u32 clear_option = clear ? CMDQ_WFE_UPDATE : 0;
>> -    if (event >= CMDQ_MAX_EVENT)
>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>           return -EINVAL;
>>       inst.op = CMDQ_CODE_WFE;
>> @@ -351,7 +351,7 @@ int cmdq_pkt_acquire_event(struct cmdq_pkt *pkt, u16 event)
>>   {
>>       struct cmdq_instruction inst = {};
>> -    if (event >= CMDQ_MAX_EVENT)
>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>           return -EINVAL;
>>       inst.op = CMDQ_CODE_WFE;
>> @@ -366,7 +366,7 @@ int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
>>   {
>>       struct cmdq_instruction inst = { {0} };
>> -    if (event >= CMDQ_MAX_EVENT)
>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>           return -EINVAL;
>>       inst.op = CMDQ_CODE_WFE;
>> @@ -381,7 +381,7 @@ int cmdq_pkt_set_event(struct cmdq_pkt *pkt, u16 event)
>>   {
>>       struct cmdq_instruction inst = {};
>> -    if (event >= CMDQ_MAX_EVENT)
>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>           return -EINVAL;
>>       inst.op = CMDQ_CODE_WFE;
>> @@ -476,7 +476,7 @@ int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, u16 
>> result_reg_idx,
>>   {
>>       struct cmdq_instruction inst = { {0} };
>> -    if (!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX)
>> +    if (unlikely(!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX))
>>           return -EINVAL;
>>       inst.op = CMDQ_CODE_LOGIC;

-- 
AngeloGioacchino Del Regno
Senior Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718

Re: [PATCH v1 2/3] soc: mediatek: mtk-cmdq: Mark very unlikely branches as such

[Matthias Brugger]

On 02/10/2024 14:43, AngeloGioacchino Del Regno wrote:
> Il 02/10/24 14:41, Matthias Brugger ha scritto:
>>
>>
>> On 18/09/2024 12:06, AngeloGioacchino Del Regno wrote:
>>> Calling cmdq packet builders with an unsupported event number,
>>> or without left/right operands (in the case of logic commands)
>>> means that the caller (another driver) wants to perform an
>>> unsupported operation, so this means that the caller must be
>>> fixed instead.
>>>
>>> Anyway, such checks are here for safety and, unless any driver
>>> bug or any kind of misconfiguration is present, will always be
>>> false so add a very unlikely hint for those.
>>>
>>> Knowing that CPUs' branch predictors (and compilers, anyway) are
>>> indeed smart about these cases, this is done mainly for human
>>> readability purposes.
>>>
>>
>> Are you really sure we need that? As you mentioned the unlikely() is probably 
>> useless as compiler and branch predictions will do the job. I don't see the 
>> complexity in the code to have this annotations for human readability.
>>
>> I would argue against using unlikely() here as, in general, it is discouraged 
>> to use it. We will just create a data point of doing things that should only 
>> be done with very good reason. I don't see the reason here, it will only 
>> confuse other developers about the use of likely() and unlikely().
>>
> 
> If you have strong opinions I have no problem dropping this.

My take would be to drop it.

> Perhaps I can add a comment stating "this is very unlikely to happen and should
> be dropped after thorough cleanup", if that's better?
> 

As these are exported functions they could be used by out-of-tree modules, so it 
could make sense to check the input parameter. Maybe transform it to 
WARN_ON(event >= CMDQ_MAX_EVENT)?

Regards,
Matthias

> Cheers!
> Angelo
> 
>> Regards,
>> Matthias
>>
>>> Signed-off-by: AngeloGioacchino Del Regno 
>>> <angelogioacchino.delregno@collabora.com>
>>> ---
>>>   drivers/soc/mediatek/mtk-cmdq-helper.c | 10 +++++-----
>>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c 
>>> b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> index 620c371fd1fc..4ffd1a35df87 100644
>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> @@ -336,7 +336,7 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool 
>>> clear)
>>>       struct cmdq_instruction inst = { {0} };
>>>       u32 clear_option = clear ? CMDQ_WFE_UPDATE : 0;
>>> -    if (event >= CMDQ_MAX_EVENT)
>>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>>           return -EINVAL;
>>>       inst.op = CMDQ_CODE_WFE;
>>> @@ -351,7 +351,7 @@ int cmdq_pkt_acquire_event(struct cmdq_pkt *pkt, u16 event)
>>>   {
>>>       struct cmdq_instruction inst = {};
>>> -    if (event >= CMDQ_MAX_EVENT)
>>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>>           return -EINVAL;
>>>       inst.op = CMDQ_CODE_WFE;
>>> @@ -366,7 +366,7 @@ int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
>>>   {
>>>       struct cmdq_instruction inst = { {0} };
>>> -    if (event >= CMDQ_MAX_EVENT)
>>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>>           return -EINVAL;
>>>       inst.op = CMDQ_CODE_WFE;
>>> @@ -381,7 +381,7 @@ int cmdq_pkt_set_event(struct cmdq_pkt *pkt, u16 event)
>>>   {
>>>       struct cmdq_instruction inst = {};
>>> -    if (event >= CMDQ_MAX_EVENT)
>>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>>           return -EINVAL;
>>>       inst.op = CMDQ_CODE_WFE;
>>> @@ -476,7 +476,7 @@ int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, u16 
>>> result_reg_idx,
>>>   {
>>>       struct cmdq_instruction inst = { {0} };
>>> -    if (!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX)
>>> +    if (unlikely(!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX))
>>>           return -EINVAL;
>>>       inst.op = CMDQ_CODE_LOGIC;
> 

Re: [PATCH v1 2/3] soc: mediatek: mtk-cmdq: Mark very unlikely branches as such

[AngeloGioacchino Del Regno]

Il 02/10/24 14:58, Matthias Brugger ha scritto:
> 
> 
> On 02/10/2024 14:43, AngeloGioacchino Del Regno wrote:
>> Il 02/10/24 14:41, Matthias Brugger ha scritto:
>>>
>>>
>>> On 18/09/2024 12:06, AngeloGioacchino Del Regno wrote:
>>>> Calling cmdq packet builders with an unsupported event number,
>>>> or without left/right operands (in the case of logic commands)
>>>> means that the caller (another driver) wants to perform an
>>>> unsupported operation, so this means that the caller must be
>>>> fixed instead.
>>>>
>>>> Anyway, such checks are here for safety and, unless any driver
>>>> bug or any kind of misconfiguration is present, will always be
>>>> false so add a very unlikely hint for those.
>>>>
>>>> Knowing that CPUs' branch predictors (and compilers, anyway) are
>>>> indeed smart about these cases, this is done mainly for human
>>>> readability purposes.
>>>>
>>>
>>> Are you really sure we need that? As you mentioned the unlikely() is probably 
>>> useless as compiler and branch predictions will do the job. I don't see the 
>>> complexity in the code to have this annotations for human readability.
>>>
>>> I would argue against using unlikely() here as, in general, it is discouraged to 
>>> use it. We will just create a data point of doing things that should only be 
>>> done with very good reason. I don't see the reason here, it will only confuse 
>>> other developers about the use of likely() and unlikely().
>>>
>>
>> If you have strong opinions I have no problem dropping this.
> 
> My take would be to drop it.
> 

Let's drop it then. :-)

>> Perhaps I can add a comment stating "this is very unlikely to happen and should
>> be dropped after thorough cleanup", if that's better?
>>
> 
> As these are exported functions they could be used by out-of-tree modules, so it 
> could make sense to check the input parameter. Maybe transform it to WARN_ON(event 
>  >= CMDQ_MAX_EVENT)?
> 

The reasoning behind all of this is that those functions are used in drivers'
performance paths, including display and camera.... so a WARN_ON would be even
more against what I'm trying to do.

At this point we can just leave this as it is....

> Regards,
> Matthias
> 
>> Cheers!
>> Angelo
>>
>>> Regards,
>>> Matthias
>>>
>>>> Signed-off-by: AngeloGioacchino Del Regno 
>>>> <angelogioacchino.delregno@collabora.com>
>>>> ---
>>>>   drivers/soc/mediatek/mtk-cmdq-helper.c | 10 +++++-----
>>>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c 
>>>> b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>>> index 620c371fd1fc..4ffd1a35df87 100644
>>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
>>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>>> @@ -336,7 +336,7 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear)
>>>>       struct cmdq_instruction inst = { {0} };
>>>>       u32 clear_option = clear ? CMDQ_WFE_UPDATE : 0;
>>>> -    if (event >= CMDQ_MAX_EVENT)
>>>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>>>           return -EINVAL;
>>>>       inst.op = CMDQ_CODE_WFE;
>>>> @@ -351,7 +351,7 @@ int cmdq_pkt_acquire_event(struct cmdq_pkt *pkt, u16 event)
>>>>   {
>>>>       struct cmdq_instruction inst = {};
>>>> -    if (event >= CMDQ_MAX_EVENT)
>>>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>>>           return -EINVAL;
>>>>       inst.op = CMDQ_CODE_WFE;
>>>> @@ -366,7 +366,7 @@ int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
>>>>   {
>>>>       struct cmdq_instruction inst = { {0} };
>>>> -    if (event >= CMDQ_MAX_EVENT)
>>>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>>>           return -EINVAL;
>>>>       inst.op = CMDQ_CODE_WFE;
>>>> @@ -381,7 +381,7 @@ int cmdq_pkt_set_event(struct cmdq_pkt *pkt, u16 event)
>>>>   {
>>>>       struct cmdq_instruction inst = {};
>>>> -    if (event >= CMDQ_MAX_EVENT)
>>>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>>>           return -EINVAL;
>>>>       inst.op = CMDQ_CODE_WFE;
>>>> @@ -476,7 +476,7 @@ int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, u16 
>>>> result_reg_idx,
>>>>   {
>>>>       struct cmdq_instruction inst = { {0} };
>>>> -    if (!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX)
>>>> +    if (unlikely(!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX))
>>>>           return -EINVAL;
>>>>       inst.op = CMDQ_CODE_LOGIC;
>>

[PATCH v1 3/3] soc: mediatek: mtk-cmdq: Move cmdq_instruction init to declaration

[AngeloGioacchino Del Regno]

Move, where possible, the initialization of struct cmdq_instruction
variables to their declaration to compress the code.

While at it, also change an instance of open-coded mask to use the
GENMASK() macro instead, and instances of `ret = func(); return ret;`
to the equivalent (but shorter) `return func()`.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/soc/mediatek/mtk-cmdq-helper.c | 200 ++++++++++++-------------
 1 file changed, 93 insertions(+), 107 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index 4ffd1a35df87..0b274b0fb44f 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -191,13 +191,12 @@ static int cmdq_pkt_mask(struct cmdq_pkt *pkt, u32 mask)
 
 int cmdq_pkt_write(struct cmdq_pkt *pkt, u8 subsys, u16 offset, u32 value)
 {
-	struct cmdq_instruction inst;
-
-	inst.op = CMDQ_CODE_WRITE;
-	inst.value = value;
-	inst.offset = offset;
-	inst.subsys = subsys;
-
+	struct cmdq_instruction inst = {
+		.op = CMDQ_CODE_WRITE,
+		.value = value,
+		.offset = offset,
+		.subsys = subsys
+	};
 	return cmdq_pkt_append_command(pkt, inst);
 }
 EXPORT_SYMBOL(cmdq_pkt_write);
@@ -208,30 +207,27 @@ int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys,
 	u16 offset_mask = offset;
 	int err;
 
-	if (mask != 0xffffffff) {
+	if (mask != GENMASK(31, 0)) {
 		err = cmdq_pkt_mask(pkt, mask);
 		if (err < 0)
 			return err;
 
 		offset_mask |= CMDQ_WRITE_ENABLE_MASK;
 	}
-	err = cmdq_pkt_write(pkt, subsys, offset_mask, value);
-
-	return err;
+	return cmdq_pkt_write(pkt, subsys, offset_mask, value);
 }
 EXPORT_SYMBOL(cmdq_pkt_write_mask);
 
 int cmdq_pkt_read_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, u16 addr_low,
 		    u16 reg_idx)
 {
-	struct cmdq_instruction inst = {};
-
-	inst.op = CMDQ_CODE_READ_S;
-	inst.dst_t = CMDQ_REG_TYPE;
-	inst.sop = high_addr_reg_idx;
-	inst.reg_dst = reg_idx;
-	inst.src_reg = addr_low;
-
+	struct cmdq_instruction inst = {
+		.op = CMDQ_CODE_READ_S,
+		.dst_t = CMDQ_REG_TYPE,
+		.sop = high_addr_reg_idx,
+		.reg_dst = reg_idx,
+		.src_reg = addr_low
+	};
 	return cmdq_pkt_append_command(pkt, inst);
 }
 EXPORT_SYMBOL(cmdq_pkt_read_s);
@@ -239,14 +235,14 @@ EXPORT_SYMBOL(cmdq_pkt_read_s);
 int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
 		     u16 addr_low, u16 src_reg_idx)
 {
-	struct cmdq_instruction inst = {};
-
-	inst.op = CMDQ_CODE_WRITE_S;
-	inst.src_t = CMDQ_REG_TYPE;
-	inst.sop = high_addr_reg_idx;
-	inst.offset = addr_low;
-	inst.src_reg = src_reg_idx;
-
+	struct cmdq_instruction inst = {
+		.op = CMDQ_CODE_WRITE_S,
+		.mask = 0,
+		.src_t = CMDQ_REG_TYPE,
+		.sop = high_addr_reg_idx,
+		.offset = addr_low,
+		.src_reg = src_reg_idx
+	};
 	return cmdq_pkt_append_command(pkt, inst);
 }
 EXPORT_SYMBOL(cmdq_pkt_write_s);
@@ -254,20 +250,19 @@ EXPORT_SYMBOL(cmdq_pkt_write_s);
 int cmdq_pkt_write_s_mask(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
 			  u16 addr_low, u16 src_reg_idx, u32 mask)
 {
-	struct cmdq_instruction inst = {};
+	struct cmdq_instruction inst = {
+		.op = CMDQ_CODE_WRITE_S_MASK,
+		.src_t = CMDQ_REG_TYPE,
+		.sop = high_addr_reg_idx,
+		.offset = addr_low,
+		.src_reg = src_reg_idx,
+	};
 	int err;
 
 	err = cmdq_pkt_mask(pkt, mask);
 	if (err < 0)
 		return err;
 
-	inst.mask = 0;
-	inst.op = CMDQ_CODE_WRITE_S_MASK;
-	inst.src_t = CMDQ_REG_TYPE;
-	inst.sop = high_addr_reg_idx;
-	inst.offset = addr_low;
-	inst.src_reg = src_reg_idx;
-
 	return cmdq_pkt_append_command(pkt, inst);
 }
 EXPORT_SYMBOL(cmdq_pkt_write_s_mask);
@@ -275,13 +270,12 @@ EXPORT_SYMBOL(cmdq_pkt_write_s_mask);
 int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
 			   u16 addr_low, u32 value)
 {
-	struct cmdq_instruction inst = {};
-
-	inst.op = CMDQ_CODE_WRITE_S;
-	inst.sop = high_addr_reg_idx;
-	inst.offset = addr_low;
-	inst.value = value;
-
+	struct cmdq_instruction inst = {
+		.op = CMDQ_CODE_WRITE_S,
+		.sop = high_addr_reg_idx,
+		.offset = addr_low,
+		.value = value
+	};
 	return cmdq_pkt_append_command(pkt, inst);
 }
 EXPORT_SYMBOL(cmdq_pkt_write_s_value);
@@ -289,18 +283,18 @@ EXPORT_SYMBOL(cmdq_pkt_write_s_value);
 int cmdq_pkt_write_s_mask_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
 				u16 addr_low, u32 value, u32 mask)
 {
-	struct cmdq_instruction inst = {};
+	struct cmdq_instruction inst = {
+		.op = CMDQ_CODE_WRITE_S_MASK,
+		.sop = high_addr_reg_idx,
+		.offset = addr_low,
+		.value = value
+	};
 	int err;
 
 	err = cmdq_pkt_mask(pkt, mask);
 	if (err < 0)
 		return err;
 
-	inst.op = CMDQ_CODE_WRITE_S_MASK;
-	inst.sop = high_addr_reg_idx;
-	inst.offset = addr_low;
-	inst.value = value;
-
 	return cmdq_pkt_append_command(pkt, inst);
 }
 EXPORT_SYMBOL(cmdq_pkt_write_s_mask_value);
@@ -333,61 +327,61 @@ EXPORT_SYMBOL(cmdq_pkt_mem_move);
 
 int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear)
 {
-	struct cmdq_instruction inst = { {0} };
 	u32 clear_option = clear ? CMDQ_WFE_UPDATE : 0;
+	struct cmdq_instruction inst = {
+		.op = CMDQ_CODE_WFE,
+		.value = CMDQ_WFE_OPTION | clear_option,
+		.event = event
+	};
 
 	if (unlikely(event >= CMDQ_MAX_EVENT))
 		return -EINVAL;
 
-	inst.op = CMDQ_CODE_WFE;
-	inst.value = CMDQ_WFE_OPTION | clear_option;
-	inst.event = event;
-
 	return cmdq_pkt_append_command(pkt, inst);
 }
 EXPORT_SYMBOL(cmdq_pkt_wfe);
 
 int cmdq_pkt_acquire_event(struct cmdq_pkt *pkt, u16 event)
 {
-	struct cmdq_instruction inst = {};
+	struct cmdq_instruction inst = {
+		.op = CMDQ_CODE_WFE,
+		.value = CMDQ_WFE_UPDATE | CMDQ_WFE_UPDATE_VALUE | CMDQ_WFE_WAIT,
+		.event = event
+	};
 
 	if (unlikely(event >= CMDQ_MAX_EVENT))
 		return -EINVAL;
 
-	inst.op = CMDQ_CODE_WFE;
-	inst.value = CMDQ_WFE_UPDATE | CMDQ_WFE_UPDATE_VALUE | CMDQ_WFE_WAIT;
-	inst.event = event;
-
 	return cmdq_pkt_append_command(pkt, inst);
 }
 EXPORT_SYMBOL(cmdq_pkt_acquire_event);
 
 int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
 {
-	struct cmdq_instruction inst = { {0} };
+	struct cmdq_instruction inst = {
+		.op = CMDQ_CODE_WFE,
+		.value = CMDQ_WFE_UPDATE,
+		.event = event
+	};
 
 	if (unlikely(event >= CMDQ_MAX_EVENT))
 		return -EINVAL;
 
-	inst.op = CMDQ_CODE_WFE;
-	inst.value = CMDQ_WFE_UPDATE;
-	inst.event = event;
-
 	return cmdq_pkt_append_command(pkt, inst);
 }
 EXPORT_SYMBOL(cmdq_pkt_clear_event);
 
 int cmdq_pkt_set_event(struct cmdq_pkt *pkt, u16 event)
 {
-	struct cmdq_instruction inst = {};
+	struct cmdq_instruction inst = {
+		.op = CMDQ_CODE_WFE,
+		.value = CMDQ_WFE_UPDATE | CMDQ_WFE_UPDATE_VALUE,
+		.event = event
+	};
 
 	if (unlikely(event >= CMDQ_MAX_EVENT))
 		return -EINVAL;
 
-	inst.op = CMDQ_CODE_WFE;
-	inst.value = CMDQ_WFE_UPDATE | CMDQ_WFE_UPDATE_VALUE;
-	inst.event = event;
-
 	return cmdq_pkt_append_command(pkt, inst);
 }
 EXPORT_SYMBOL(cmdq_pkt_set_event);
@@ -395,16 +389,13 @@ EXPORT_SYMBOL(cmdq_pkt_set_event);
 int cmdq_pkt_poll(struct cmdq_pkt *pkt, u8 subsys,
 		  u16 offset, u32 value)
 {
-	struct cmdq_instruction inst = { {0} };
-	int err;
-
-	inst.op = CMDQ_CODE_POLL;
-	inst.value = value;
-	inst.offset = offset;
-	inst.subsys = subsys;
-	err = cmdq_pkt_append_command(pkt, inst);
-
-	return err;
+	struct cmdq_instruction inst = {
+		.op = CMDQ_CODE_POLL,
+		.value = value,
+		.offset = offset,
+		.subsys = subsys
+	};
+	return cmdq_pkt_append_command(pkt, inst);
 }
 EXPORT_SYMBOL(cmdq_pkt_poll);
 
@@ -418,9 +409,7 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
 		return err;
 
 	offset = offset | CMDQ_POLL_ENABLE_MASK;
-	err = cmdq_pkt_poll(pkt, subsys, offset, value);
-
-	return err;
+	return cmdq_pkt_poll(pkt, subsys, offset, value);
 }
 EXPORT_SYMBOL(cmdq_pkt_poll_mask);
 
@@ -474,11 +463,12 @@ int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, u16 result_reg_idx,
 			   enum cmdq_logic_op s_op,
 			   struct cmdq_operand *right_operand)
 {
-	struct cmdq_instruction inst = { {0} };
+	struct cmdq_instruction inst;
 
 	if (unlikely(!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX))
 		return -EINVAL;
 
+	inst.value = 0;
 	inst.op = CMDQ_CODE_LOGIC;
 	inst.dst_t = CMDQ_REG_TYPE;
 	inst.src_t = cmdq_operand_get_type(left_operand);
@@ -494,43 +484,43 @@ EXPORT_SYMBOL(cmdq_pkt_logic_command);
 
 int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
 {
-	struct cmdq_instruction inst = {};
-
-	inst.op = CMDQ_CODE_LOGIC;
-	inst.dst_t = CMDQ_REG_TYPE;
-	inst.reg_dst = reg_idx;
-	inst.value = value;
+	struct cmdq_instruction inst = {
+		.op = CMDQ_CODE_LOGIC,
+		.dst_t = CMDQ_REG_TYPE,
+		.reg_dst = reg_idx,
+		.value = value
+	};
 	return cmdq_pkt_append_command(pkt, inst);
 }
 EXPORT_SYMBOL(cmdq_pkt_assign);
 
 int cmdq_pkt_jump_abs(struct cmdq_pkt *pkt, dma_addr_t addr, u8 shift_pa)
 {
-	struct cmdq_instruction inst = {};
-
-	inst.op = CMDQ_CODE_JUMP;
-	inst.offset = CMDQ_JUMP_ABSOLUTE;
-	inst.value = addr >> shift_pa;
+	struct cmdq_instruction inst = {
+		.op = CMDQ_CODE_JUMP,
+		.offset = CMDQ_JUMP_ABSOLUTE,
+		.value = addr >> shift_pa
+	};
 	return cmdq_pkt_append_command(pkt, inst);
 }
 EXPORT_SYMBOL(cmdq_pkt_jump_abs);
 
 int cmdq_pkt_jump_rel(struct cmdq_pkt *pkt, s32 offset, u8 shift_pa)
 {
-	struct cmdq_instruction inst = { {0} };
-
-	inst.op = CMDQ_CODE_JUMP;
-	inst.value = (u32)offset >> shift_pa;
+	struct cmdq_instruction inst = {
+		.op = CMDQ_CODE_JUMP,
+		.value = (u32)offset >> shift_pa
+	};
 	return cmdq_pkt_append_command(pkt, inst);
 }
 EXPORT_SYMBOL(cmdq_pkt_jump_rel);
 
 int cmdq_pkt_eoc(struct cmdq_pkt *pkt)
 {
-	struct cmdq_instruction inst = { {0} };
-
-	inst.op = CMDQ_CODE_EOC;
-	inst.value = CMDQ_EOC_IRQ_EN;
+	struct cmdq_instruction inst = {
+		.op = CMDQ_CODE_EOC,
+		.value = CMDQ_EOC_IRQ_EN
+	};
 	return cmdq_pkt_append_command(pkt, inst);
 }
 EXPORT_SYMBOL(cmdq_pkt_eoc);
@@ -541,9 +531,7 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
 	int err;
 
 	/* insert EOC and generate IRQ for each command iteration */
-	inst.op = CMDQ_CODE_EOC;
-	inst.value = CMDQ_EOC_IRQ_EN;
-	err = cmdq_pkt_append_command(pkt, inst);
+	err = cmdq_pkt_eoc(pkt);
 	if (err < 0)
 		return err;
 
@@ -551,9 +539,7 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
 	inst.op = CMDQ_CODE_JUMP;
 	inst.value = CMDQ_JUMP_PASS >>
 		cmdq_get_shift_pa(((struct cmdq_client *)pkt->cl)->chan);
-	err = cmdq_pkt_append_command(pkt, inst);
-
-	return err;
+	return cmdq_pkt_append_command(pkt, inst);
 }
 EXPORT_SYMBOL(cmdq_pkt_finalize);
 
-- 
2.46.0

Re: [PATCH v1 3/3] soc: mediatek: mtk-cmdq: Move cmdq_instruction init to declaration

[kernel test robot]

Hi AngeloGioacchino,

kernel test robot noticed the following build warnings:

[auto build test WARNING on clk/clk-next]
[also build test WARNING on soc/for-next linus/master v6.11 next-20240919]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/AngeloGioacchino-Del-Regno/soc-mediatek-mtk-cmdq-Move-mask-build-and-append-to-function/20240918-180757
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
patch link:    https://lore.kernel.org/r/20240918100620.103536-4-angelogioacchino.delregno%40collabora.com
patch subject: [PATCH v1 3/3] soc: mediatek: mtk-cmdq: Move cmdq_instruction init to declaration
config: parisc-randconfig-r123-20240920 (https://download.01.org/0day-ci/archive/20240920/202409200659.IVYRJ33l-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 14.1.0
reproduce: (https://download.01.org/0day-ci/archive/20240920/202409200659.IVYRJ33l-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409200659.IVYRJ33l-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/soc/mediatek/mtk-cmdq-helper.c:240:18: sparse: sparse: Initializer entry defined twice
   drivers/soc/mediatek/mtk-cmdq-helper.c:244:18: sparse:   also defined here

vim +240 drivers/soc/mediatek/mtk-cmdq-helper.c

   234	
   235	int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
   236			     u16 addr_low, u16 src_reg_idx)
   237	{
   238		struct cmdq_instruction inst = {
   239			.op = CMDQ_CODE_WRITE_S,
 > 240			.mask = 0,
   241			.src_t = CMDQ_REG_TYPE,
   242			.sop = high_addr_reg_idx,
   243			.offset = addr_low,
   244			.src_reg = src_reg_idx
   245		};
   246		return cmdq_pkt_append_command(pkt, inst);
   247	}
   248	EXPORT_SYMBOL(cmdq_pkt_write_s);
   249	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 3/3] soc: mediatek: mtk-cmdq: Move cmdq_instruction init to declaration

[Matthias Brugger]

On 18/09/2024 12:06, AngeloGioacchino Del Regno wrote:
> Move, where possible, the initialization of struct cmdq_instruction
> variables to their declaration to compress the code.
> 
> While at it, also change an instance of open-coded mask to use the
> GENMASK() macro instead, and instances of `ret = func(); return ret;`
> to the equivalent (but shorter) `return func()`.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/soc/mediatek/mtk-cmdq-helper.c | 200 ++++++++++++-------------
>   1 file changed, 93 insertions(+), 107 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index 4ffd1a35df87..0b274b0fb44f 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
[...]
> @@ -474,11 +463,12 @@ int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, u16 result_reg_idx,
>   			   enum cmdq_logic_op s_op,
>   			   struct cmdq_operand *right_operand)
>   {
> -	struct cmdq_instruction inst = { {0} };
> +	struct cmdq_instruction inst;
>   
>   	if (unlikely(!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX))
>   		return -EINVAL;
>   
> +	inst.value = 0;
>   	inst.op = CMDQ_CODE_LOGIC;
>   	inst.dst_t = CMDQ_REG_TYPE;

I would add all members that are not based on left and right operand to the 
definition of the struct.

Regards,
Matthias

Re: [PATCH v1 0/3] soc: mediatek: mtk-cmdq-helper: Various cleanups

[AngeloGioacchino Del Regno]

On Wed, 18 Sep 2024 12:06:17 +0200, AngeloGioacchino Del Regno wrote:
> This series performs various cleanups to the MediaTek CMDQ Helper lib,
> reducing code duplication and enhancing human readability.
> 
> This also avoids double initialization struct cmdq_instruction as,
> in some cases, it was stack-initialized to zero and then overwritten
> completely anyway a bit later.
> I'd expect compilers to be somehow smart about that, but still, while
> at it ... why not :-)
> 
> [...]

Applied to v6.12-next/soc, thanks!

[1/3] soc: mediatek: mtk-cmdq: Move mask build and append to function
      https://git.kernel.org/mediatek/c/2400e830
[2/3] soc: mediatek: mtk-cmdq: Mark very unlikely branches as such
      https://git.kernel.org/mediatek/c/21ab3dae
[3/3] soc: mediatek: mtk-cmdq: Move cmdq_instruction init to declaration
      https://git.kernel.org/mediatek/c/66705b89

Cheers,
Angelo

Re: [PATCH v1 0/3] soc: mediatek: mtk-cmdq-helper: Various cleanups

[Matthias Brugger]

On 02/10/2024 11:08, AngeloGioacchino Del Regno wrote:
> On Wed, 18 Sep 2024 12:06:17 +0200, AngeloGioacchino Del Regno wrote:
>> This series performs various cleanups to the MediaTek CMDQ Helper lib,
>> reducing code duplication and enhancing human readability.
>>
>> This also avoids double initialization struct cmdq_instruction as,
>> in some cases, it was stack-initialized to zero and then overwritten
>> completely anyway a bit later.
>> I'd expect compilers to be somehow smart about that, but still, while
>> at it ... why not :-)
>>
>> [...]
> 
> Applied to v6.12-next/soc, thanks!
> 
> [1/3] soc: mediatek: mtk-cmdq: Move mask build and append to function
>        https://git.kernel.org/mediatek/c/2400e830
> [2/3] soc: mediatek: mtk-cmdq: Mark very unlikely branches as such
>        https://git.kernel.org/mediatek/c/21ab3dae
> [3/3] soc: mediatek: mtk-cmdq: Move cmdq_instruction init to declaration
>        https://git.kernel.org/mediatek/c/66705b89
> 

You probably oversaw the sparse warning email on 3/3?

As I oversaw that you already merged this.

Regards,
Matthias

Re: [PATCH v1 0/3] soc: mediatek: mtk-cmdq-helper: Various cleanups

[AngeloGioacchino Del Regno]

Il 02/10/24 15:00, Matthias Brugger ha scritto:
> 
> 
> On 02/10/2024 11:08, AngeloGioacchino Del Regno wrote:
>> On Wed, 18 Sep 2024 12:06:17 +0200, AngeloGioacchino Del Regno wrote:
>>> This series performs various cleanups to the MediaTek CMDQ Helper lib,
>>> reducing code duplication and enhancing human readability.
>>>
>>> This also avoids double initialization struct cmdq_instruction as,
>>> in some cases, it was stack-initialized to zero and then overwritten
>>> completely anyway a bit later.
>>> I'd expect compilers to be somehow smart about that, but still, while
>>> at it ... why not :-)
>>>
>>> [...]
>>
>> Applied to v6.12-next/soc, thanks!
>>
>> [1/3] soc: mediatek: mtk-cmdq: Move mask build and append to function
>>        https://git.kernel.org/mediatek/c/2400e830
>> [2/3] soc: mediatek: mtk-cmdq: Mark very unlikely branches as such
>>        https://git.kernel.org/mediatek/c/21ab3dae
>> [3/3] soc: mediatek: mtk-cmdq: Move cmdq_instruction init to declaration
>>        https://git.kernel.org/mediatek/c/66705b89
>>
> 
> You probably oversaw the sparse warning email on 3/3?
> 
> As I oversaw that you already merged this.
> 

Yeah, I did. There's no problem anyway, as we can always replace the commits
without noise, I haven't pushed anything to -next :-)

I plan to do that (drop patch 2/3, delete `.mask = 0` on 3/3) soon, at max
next week anyway.

Cheers,
Angelo

Re: [PATCH v1 0/3] soc: mediatek: mtk-cmdq-helper: Various cleanups

[Matthias Brugger]

On 03/10/2024 10:08, AngeloGioacchino Del Regno wrote:
> Il 02/10/24 15:00, Matthias Brugger ha scritto:
>>
>>
>> On 02/10/2024 11:08, AngeloGioacchino Del Regno wrote:
>>> On Wed, 18 Sep 2024 12:06:17 +0200, AngeloGioacchino Del Regno wrote:
>>>> This series performs various cleanups to the MediaTek CMDQ Helper lib,
>>>> reducing code duplication and enhancing human readability.
>>>>
>>>> This also avoids double initialization struct cmdq_instruction as,
>>>> in some cases, it was stack-initialized to zero and then overwritten
>>>> completely anyway a bit later.
>>>> I'd expect compilers to be somehow smart about that, but still, while
>>>> at it ... why not :-)
>>>>
>>>> [...]
>>>
>>> Applied to v6.12-next/soc, thanks!
>>>
>>> [1/3] soc: mediatek: mtk-cmdq: Move mask build and append to function
>>>        https://git.kernel.org/mediatek/c/2400e830
>>> [2/3] soc: mediatek: mtk-cmdq: Mark very unlikely branches as such
>>>        https://git.kernel.org/mediatek/c/21ab3dae
>>> [3/3] soc: mediatek: mtk-cmdq: Move cmdq_instruction init to declaration
>>>        https://git.kernel.org/mediatek/c/66705b89
>>>
>>
>> You probably oversaw the sparse warning email on 3/3?
>>
>> As I oversaw that you already merged this.
>>
> 
> Yeah, I did. There's no problem anyway, as we can always replace the commits
> without noise, I haven't pushed anything to -next :-)
> 
> I plan to do that (drop patch 2/3, delete `.mask = 0` on 3/3) soon, at max
> next week anyway.
> 

Perfect, before you do any locally please pull from remote as I did some 
changes, not sure if you saw my message on IRC.

Regarding -next: we don't need to keep commit history in linux-next neither, to 
even it got pushed to -next, we can drop patches in the queue with any problem.

Have a nice afternoon :)
Matthias

Re: [PATCH v1 0/3] soc: mediatek: mtk-cmdq-helper: Various cleanups

[AngeloGioacchino Del Regno]

Il 07/10/24 16:33, Matthias Brugger ha scritto:
> 
> 
> On 03/10/2024 10:08, AngeloGioacchino Del Regno wrote:
>> Il 02/10/24 15:00, Matthias Brugger ha scritto:
>>>
>>>
>>> On 02/10/2024 11:08, AngeloGioacchino Del Regno wrote:
>>>> On Wed, 18 Sep 2024 12:06:17 +0200, AngeloGioacchino Del Regno wrote:
>>>>> This series performs various cleanups to the MediaTek CMDQ Helper lib,
>>>>> reducing code duplication and enhancing human readability.
>>>>>
>>>>> This also avoids double initialization struct cmdq_instruction as,
>>>>> in some cases, it was stack-initialized to zero and then overwritten
>>>>> completely anyway a bit later.
>>>>> I'd expect compilers to be somehow smart about that, but still, while
>>>>> at it ... why not :-)
>>>>>
>>>>> [...]
>>>>
>>>> Applied to v6.12-next/soc, thanks!
>>>>
>>>> [1/3] soc: mediatek: mtk-cmdq: Move mask build and append to function
>>>>        https://git.kernel.org/mediatek/c/2400e830
>>>> [2/3] soc: mediatek: mtk-cmdq: Mark very unlikely branches as such
>>>>        https://git.kernel.org/mediatek/c/21ab3dae
>>>> [3/3] soc: mediatek: mtk-cmdq: Move cmdq_instruction init to declaration
>>>>        https://git.kernel.org/mediatek/c/66705b89
>>>>
>>>
>>> You probably oversaw the sparse warning email on 3/3?
>>>
>>> As I oversaw that you already merged this.
>>>
>>
>> Yeah, I did. There's no problem anyway, as we can always replace the commits
>> without noise, I haven't pushed anything to -next :-)
>>
>> I plan to do that (drop patch 2/3, delete `.mask = 0` on 3/3) soon, at max
>> next week anyway.
>>
> 
> Perfect, before you do any locally please pull from remote as I did some changes, 
> not sure if you saw my message on IRC.
> 
> Regarding -next: we don't need to keep commit history in linux-next neither, to 
> even it got pushed to -next, we can drop patches in the queue with any problem.

My day was pretty full, I didn't. Thanks for shooting the email :-D

> 
> Have a nice afternoon :)

You too!
Angelo