diff mbox series

[v2,5/7] vc04_services: vchiq_arm: Drop VCHIQ_RETRY usage on disconnect

Message ID 20221219115725.11992-6-umang.jain@ideasonboard.com
State New
Headers
Series staging: vc04_services: Remove custom return values |

Commit Message

Umang Jain Dec. 19, 2022, 11:57 a.m. UTC 
  Drop the usage of VCHIQ_RETRY when the vchiq has connection status
VCHIQ_CONNSTATE_DISCONNECTED. Disconnected status will not be valid to
carry on a retry, replace the VCHIQ_RETRY with -ENOTCONN.

This patch removes the usage of vCHIQ_RETRY completely and act as
intermediatory to address the TODO item:
	* Get rid of custom function return values
for vc04_services/interface.

Fixes: 71bad7f08641 ("staging: add bcm2708 vchiq driver")
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../staging/vc04_services/interface/vchiq_arm/vchiq_core.c    | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Stefan Wahren Dec. 22, 2022, 11:07 a.m. UTC | #1 
Hi Umang,

Am 19.12.22 um 12:57 schrieb Umang Jain:
> Drop the usage of VCHIQ_RETRY when the vchiq has connection status
> VCHIQ_CONNSTATE_DISCONNECTED. Disconnected status will not be valid to
> carry on a retry, replace the VCHIQ_RETRY with -ENOTCONN.
>
> This patch removes the usage of vCHIQ_RETRY completely and act as
> intermediatory to address the TODO item:
> 	* Get rid of custom function return values
> for vc04_services/interface.
>
> Fixes: 71bad7f08641 ("staging: add bcm2708 vchiq driver")
please drop this fixes tag since this commit doesn't fix a real issue 
and also shouldn't be applied to stable.
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>   .../staging/vc04_services/interface/vchiq_arm/vchiq_core.c    | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> index 9c64d5de810e..ddb6d0f4daed 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -3641,7 +3641,7 @@ vchiq_loud_error_footer(void)
>   int vchiq_send_remote_use(struct vchiq_state *state)
>   {
>   	if (state->conn_state == VCHIQ_CONNSTATE_DISCONNECTED)
> -		return VCHIQ_RETRY;
> +		return -ENOTCONN;
>   
>   	return queue_message(state, NULL, MAKE_REMOTE_USE, NULL, NULL, 0, 0);
>   }
> @@ -3649,7 +3649,7 @@ int vchiq_send_remote_use(struct vchiq_state *state)
>   int vchiq_send_remote_use_active(struct vchiq_state *state)
>   {
>   	if (state->conn_state == VCHIQ_CONNSTATE_DISCONNECTED)
> -		return VCHIQ_RETRY;
> +		return -ENOTCONN;
>   
>   	return queue_message(state, NULL, MAKE_REMOTE_USE_ACTIVE,
>   			     NULL, NULL, 0, 0);
Umang Jain Dec. 22, 2022, 12:04 p.m. UTC | #2 
Hi Stefan,

On 12/22/22 4:37 PM, Stefan Wahren wrote:
> Hi Umang,
>
> Am 19.12.22 um 12:57 schrieb Umang Jain:
>> Drop the usage of VCHIQ_RETRY when the vchiq has connection status
>> VCHIQ_CONNSTATE_DISCONNECTED. Disconnected status will not be valid to
>> carry on a retry, replace the VCHIQ_RETRY with -ENOTCONN.
>>
>> This patch removes the usage of vCHIQ_RETRY completely and act as
>> intermediatory to address the TODO item:
>>     * Get rid of custom function return values
>> for vc04_services/interface.
>>
>> Fixes: 71bad7f08641 ("staging: add bcm2708 vchiq driver")
> please drop this fixes tag since this commit doesn't fix a real issue 
> and also shouldn't be applied to stable.

Should I send a v3 of the series with updated commit message or can you 
drop the tag while applying?

Other option would be to send v2.1  --in-reply-to this patch. I am fine 
with anything as long as it aligns with the merging workflow.

Thanks,
Umang
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   .../staging/vc04_services/interface/vchiq_arm/vchiq_core.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git 
>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c 
>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> index 9c64d5de810e..ddb6d0f4daed 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> @@ -3641,7 +3641,7 @@ vchiq_loud_error_footer(void)
>>   int vchiq_send_remote_use(struct vchiq_state *state)
>>   {
>>       if (state->conn_state == VCHIQ_CONNSTATE_DISCONNECTED)
>> -        return VCHIQ_RETRY;
>> +        return -ENOTCONN;
>>         return queue_message(state, NULL, MAKE_REMOTE_USE, NULL, 
>> NULL, 0, 0);
>>   }
>> @@ -3649,7 +3649,7 @@ int vchiq_send_remote_use(struct vchiq_state 
>> *state)
>>   int vchiq_send_remote_use_active(struct vchiq_state *state)
>>   {
>>       if (state->conn_state == VCHIQ_CONNSTATE_DISCONNECTED)
>> -        return VCHIQ_RETRY;
>> +        return -ENOTCONN;
>>         return queue_message(state, NULL, MAKE_REMOTE_USE_ACTIVE,
>>                    NULL, NULL, 0, 0);
Stefan Wahren Dec. 22, 2022, 12:27 p.m. UTC | #3 
Hi Umang,

Am 22.12.22 um 13:04 schrieb Umang Jain:
> Hi Stefan,
>
> On 12/22/22 4:37 PM, Stefan Wahren wrote:
>> Hi Umang,
>>
>> Am 19.12.22 um 12:57 schrieb Umang Jain:
>>> Drop the usage of VCHIQ_RETRY when the vchiq has connection status
>>> VCHIQ_CONNSTATE_DISCONNECTED. Disconnected status will not be valid to
>>> carry on a retry, replace the VCHIQ_RETRY with -ENOTCONN.
>>>
>>> This patch removes the usage of vCHIQ_RETRY completely and act as
>>> intermediatory to address the TODO item:
>>>     * Get rid of custom function return values
>>> for vc04_services/interface.
>>>
>>> Fixes: 71bad7f08641 ("staging: add bcm2708 vchiq driver")
>> please drop this fixes tag since this commit doesn't fix a real issue 
>> and also shouldn't be applied to stable.
>
> Should I send a v3 of the series with updated commit message or can 
> you drop the tag while applying?

Greg is the maintainer of staging, so i cannot decide. But i would 
prefer a v3 with my tested tag added and this fixes tag dropped.

Best regards

>
> Other option would be to send v2.1  --in-reply-to this patch. I am 
> fine with anything as long as it aligns with the merging workflow.
>
> Thanks,
> Umang
>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>> ---
>>>   .../staging/vc04_services/interface/vchiq_arm/vchiq_core.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git 
>>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c 
>>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>>> index 9c64d5de810e..ddb6d0f4daed 100644
>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>>> @@ -3641,7 +3641,7 @@ vchiq_loud_error_footer(void)
>>>   int vchiq_send_remote_use(struct vchiq_state *state)
>>>   {
>>>       if (state->conn_state == VCHIQ_CONNSTATE_DISCONNECTED)
>>> -        return VCHIQ_RETRY;
>>> +        return -ENOTCONN;
>>>         return queue_message(state, NULL, MAKE_REMOTE_USE, NULL, 
>>> NULL, 0, 0);
>>>   }
>>> @@ -3649,7 +3649,7 @@ int vchiq_send_remote_use(struct vchiq_state 
>>> *state)
>>>   int vchiq_send_remote_use_active(struct vchiq_state *state)
>>>   {
>>>       if (state->conn_state == VCHIQ_CONNSTATE_DISCONNECTED)
>>> -        return VCHIQ_RETRY;
>>> +        return -ENOTCONN;
>>>         return queue_message(state, NULL, MAKE_REMOTE_USE_ACTIVE,
>>>                    NULL, NULL, 0, 0);
>
Dan Carpenter Dec. 23, 2022, 3:25 p.m. UTC | #4 
On Thu, Dec 22, 2022 at 12:07:56PM +0100, Stefan Wahren wrote:
> Hi Umang,
> 
> Am 19.12.22 um 12:57 schrieb Umang Jain:
> > Drop the usage of VCHIQ_RETRY when the vchiq has connection status
> > VCHIQ_CONNSTATE_DISCONNECTED. Disconnected status will not be valid to
> > carry on a retry, replace the VCHIQ_RETRY with -ENOTCONN.
> > 
> > This patch removes the usage of vCHIQ_RETRY completely and act as
> > intermediatory to address the TODO item:
> > 	* Get rid of custom function return values
> > for vc04_services/interface.
> > 
> > Fixes: 71bad7f08641 ("staging: add bcm2708 vchiq driver")
> please drop this fixes tag since this commit doesn't fix a real issue and
> also shouldn't be applied to stable.

I asked Umang to add the Fixes tag based on the patch description and
based on that it seemed like a behavior change just from looking at
the patch.

But actually you are right that the fixes tag is not required.

The vchiq_send_remote_use() function is never called (dead code).  The
vchiq_send_remote_use_active() function now returns a mix of custom
error codes and negative error codes.  (Ugly).  The caller only tests
for VCHIQ_SUCCESS so this code does not change behavior.  I really feel
like the commit description needs to be clearer on this.

The fixes tag is not really about stable.  Stable uses the tag, but the
fixes tag is appropriate whenever there is a bug fix.

regards,
dan carpenter
diff mbox series

Patch

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 9c64d5de810e..ddb6d0f4daed 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3641,7 +3641,7 @@  vchiq_loud_error_footer(void)
 int vchiq_send_remote_use(struct vchiq_state *state)
 {
 	if (state->conn_state == VCHIQ_CONNSTATE_DISCONNECTED)
-		return VCHIQ_RETRY;
+		return -ENOTCONN;
 
 	return queue_message(state, NULL, MAKE_REMOTE_USE, NULL, NULL, 0, 0);
 }
@@ -3649,7 +3649,7 @@  int vchiq_send_remote_use(struct vchiq_state *state)
 int vchiq_send_remote_use_active(struct vchiq_state *state)
 {
 	if (state->conn_state == VCHIQ_CONNSTATE_DISCONNECTED)
-		return VCHIQ_RETRY;
+		return -ENOTCONN;
 
 	return queue_message(state, NULL, MAKE_REMOTE_USE_ACTIVE,
 			     NULL, NULL, 0, 0);