[05/17] media: atomisp: pci: fix inverted error check for ia_css_mipi_is_source_port_valid()

Message ID 20211017161958.44351-6-kitakar@gmail.com (mailing list archive)
State Accepted, archived
Headers
Series various fixes for atomisp to make it work |

Commit Message

Tsuchiya Yuto Oct. 17, 2021, 4:19 p.m. UTC
  The function ia_css_mipi_is_source_port_valid() returns true if the port
is valid. So, we can't use the existing err variable as is.

To fix this issue while reusing that variable, invert the return value
when assigning it to the variable.

Fixes: 3c0538fbad9f ("media: atomisp: get rid of most checks for ISP2401 version")
Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
 .../staging/media/atomisp/pci/sh_css_mipi.c   | 24 ++++++++++++-------
 1 file changed, 15 insertions(+), 9 deletions(-)
  

Comments

Dan Carpenter Nov. 2, 2021, 11:33 a.m. UTC | #1
On Mon, Oct 18, 2021 at 01:19:45AM +0900, Tsuchiya Yuto wrote:
> The function ia_css_mipi_is_source_port_valid() returns true if the port
> is valid. So, we can't use the existing err variable as is.
> 
> To fix this issue while reusing that variable, invert the return value
> when assigning it to the variable.
> 
> Fixes: 3c0538fbad9f ("media: atomisp: get rid of most checks for ISP2401 version")
> Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> ---
>  .../staging/media/atomisp/pci/sh_css_mipi.c   | 24 ++++++++++++-------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> index 65fc93c5d56b..c1f2f6151c5f 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> @@ -423,10 +423,12 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
>  		return 0; /* AM TODO: Check  */
>  	}
>  
> -	if (!IS_ISP2401)
> +	if (!IS_ISP2401) {
>  		port = (unsigned int)pipe->stream->config.source.port.port;
> -	else
> -		err = ia_css_mipi_is_source_port_valid(pipe, &port);
> +	} else {
> +		/* Returns true if port is valid. So, invert it */
> +		err = !ia_css_mipi_is_source_port_valid(pipe, &port);

Don't invert it...  This isn't supposed to return 1 on failure it's
supposed to return negative error codes.


> +	}
>  
>  	assert(port < N_CSI_PORTS);
>  
> @@ -553,10 +555,12 @@ free_mipi_frames(struct ia_css_pipe *pipe)
>  			return err;
>  		}
>  
> -		if (!IS_ISP2401)
> +		if (!IS_ISP2401) {
>  			port = (unsigned int)pipe->stream->config.source.port.port;
> -		else
> -			err = ia_css_mipi_is_source_port_valid(pipe, &port);
> +		} else {
> +			/* Returns true if port is valid. So, invert it */
> +			err = !ia_css_mipi_is_source_port_valid(pipe, &port);

Presumably the same here?

> +		}
>  
>  		assert(port < N_CSI_PORTS);
>  
> @@ -665,10 +669,12 @@ send_mipi_frames(struct ia_css_pipe *pipe)
>  		/* TODO: AM: maybe this should be returning an error. */
>  	}
>  
> -	if (!IS_ISP2401)
> +	if (!IS_ISP2401) {
>  		port = (unsigned int)pipe->stream->config.source.port.port;
> -	else
> -		err = ia_css_mipi_is_source_port_valid(pipe, &port);
> +	} else {
> +		/* Returns true if port is valid. So, invert it */
> +		err = !ia_css_mipi_is_source_port_valid(pipe, &port);

Same?

> +	}
>  
>  	assert(port < N_CSI_PORTS);

regards,
dan carpenter
  
Tsuchiya Yuto Nov. 8, 2021, 3 p.m. UTC | #2
On Tue, 2021-11-02 at 14:33 +0300, Dan Carpenter wrote:
> On Mon, Oct 18, 2021 at 01:19:45AM +0900, Tsuchiya Yuto wrote:
> > The function ia_css_mipi_is_source_port_valid() returns true if the port
> > is valid. So, we can't use the existing err variable as is.
> > 
> > To fix this issue while reusing that variable, invert the return value
> > when assigning it to the variable.
> > 
> > Fixes: 3c0538fbad9f ("media: atomisp: get rid of most checks for ISP2401 version")
> > Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> > ---
> >  .../staging/media/atomisp/pci/sh_css_mipi.c   | 24 ++++++++++++-------
> >  1 file changed, 15 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > index 65fc93c5d56b..c1f2f6151c5f 100644
> > --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > @@ -423,10 +423,12 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
> >  		return 0; /* AM TODO: Check  */
> >  	}
> >  
> > -	if (!IS_ISP2401)
> > +	if (!IS_ISP2401) {
> >  		port = (unsigned int)pipe->stream->config.source.port.port;
> > -	else
> > -		err = ia_css_mipi_is_source_port_valid(pipe, &port);
> > +	} else {
> > +		/* Returns true if port is valid. So, invert it */
> > +		err = !ia_css_mipi_is_source_port_valid(pipe, &port);
> 
> Don't invert it...  This isn't supposed to return 1 on failure it's
> supposed to return negative error codes.

You mean I should instead modify the return value of
ia_css_mipi_is_source_port_valid() ?

Yeah, I also thought that the current true/false return value was a little
bit confusing.

In another words, should the function return:

    - negative values (maybe -EINVAL for this case) for invalid case
    - 0 for valid case

instead? and if we go this way, we should also rename the function name
like

    - check_ia_css_mipi_source_port_validity

or something. How does this sound?

Regards,
Tsuchiya Yuto
  
Dan Carpenter Nov. 8, 2021, 3:14 p.m. UTC | #3
On Tue, Nov 09, 2021 at 12:00:29AM +0900, Tsuchiya Yuto wrote:
> On Tue, 2021-11-02 at 14:33 +0300, Dan Carpenter wrote:
> > On Mon, Oct 18, 2021 at 01:19:45AM +0900, Tsuchiya Yuto wrote:
> > > The function ia_css_mipi_is_source_port_valid() returns true if the port
> > > is valid. So, we can't use the existing err variable as is.
> > > 
> > > To fix this issue while reusing that variable, invert the return value
> > > when assigning it to the variable.
> > > 
> > > Fixes: 3c0538fbad9f ("media: atomisp: get rid of most checks for ISP2401 version")
> > > Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> > > ---
> > >  .../staging/media/atomisp/pci/sh_css_mipi.c   | 24 ++++++++++++-------
> > >  1 file changed, 15 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > index 65fc93c5d56b..c1f2f6151c5f 100644
> > > --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > @@ -423,10 +423,12 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
> > >  		return 0; /* AM TODO: Check  */
> > >  	}
> > >  
> > > -	if (!IS_ISP2401)
> > > +	if (!IS_ISP2401) {
> > >  		port = (unsigned int)pipe->stream->config.source.port.port;
> > > -	else
> > > -		err = ia_css_mipi_is_source_port_valid(pipe, &port);
> > > +	} else {
> > > +		/* Returns true if port is valid. So, invert it */
> > > +		err = !ia_css_mipi_is_source_port_valid(pipe, &port);
> > 
> > Don't invert it...  This isn't supposed to return 1 on failure it's
> > supposed to return negative error codes.
> 
> You mean I should instead modify the return value of
> ia_css_mipi_is_source_port_valid() ?
> 

No.  ia_css_mipi_is_source_port_valid() is fine.  It has a boolean name
so returning bool is fine.  What I'm saying is that allocate_mipi_frames()
should do:

	if (!ia_css_mipi_is_source_port_valid(pipe, &port))
		err = -EINVAL;

Otherwise it returns negative error codes and 1 on failure.

regards,
dan carpenter
  
Tsuchiya Yuto Nov. 8, 2021, 3:25 p.m. UTC | #4
<removed Alan from Cc as the mail address not reachable>

On Mon, 2021-11-08 at 18:14 +0300, Dan Carpenter wrote:
> On Tue, Nov 09, 2021 at 12:00:29AM +0900, Tsuchiya Yuto wrote:
> > On Tue, 2021-11-02 at 14:33 +0300, Dan Carpenter wrote:
> > > On Mon, Oct 18, 2021 at 01:19:45AM +0900, Tsuchiya Yuto wrote:
> > > > The function ia_css_mipi_is_source_port_valid() returns true if the port
> > > > is valid. So, we can't use the existing err variable as is.
> > > > 
> > > > To fix this issue while reusing that variable, invert the return value
> > > > when assigning it to the variable.
> > > > 
> > > > Fixes: 3c0538fbad9f ("media: atomisp: get rid of most checks for ISP2401 version")
> > > > Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> > > > ---
> > > >  .../staging/media/atomisp/pci/sh_css_mipi.c   | 24 ++++++++++++-------
> > > >  1 file changed, 15 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > > index 65fc93c5d56b..c1f2f6151c5f 100644
> > > > --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > > +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > > @@ -423,10 +423,12 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
> > > >  		return 0; /* AM TODO: Check  */
> > > >  	}
> > > >  
> > > > -	if (!IS_ISP2401)
> > > > +	if (!IS_ISP2401) {
> > > >  		port = (unsigned int)pipe->stream->config.source.port.port;
> > > > -	else
> > > > -		err = ia_css_mipi_is_source_port_valid(pipe, &port);
> > > > +	} else {
> > > > +		/* Returns true if port is valid. So, invert it */
> > > > +		err = !ia_css_mipi_is_source_port_valid(pipe, &port);
> > > 
> > > Don't invert it...  This isn't supposed to return 1 on failure it's
> > > supposed to return negative error codes.
> > 
> > You mean I should instead modify the return value of
> > ia_css_mipi_is_source_port_valid() ?
> > 
> 
> No.  ia_css_mipi_is_source_port_valid() is fine.  It has a boolean name
> so returning bool is fine.  What I'm saying is that allocate_mipi_frames()
> should do:
> 
> 	if (!ia_css_mipi_is_source_port_valid(pipe, &port))
> 		err = -EINVAL;
> 
> Otherwise it returns negative error codes and 1 on failure.

Ah, I see! Thank you. I feel I'm a stupid... I'll do so in v2.

Regards,
Tsuchiya Yuto
  
Dan Carpenter Nov. 8, 2021, 3:33 p.m. UTC | #5
On Tue, Nov 09, 2021 at 12:25:52AM +0900, Tsuchiya Yuto wrote:
> 
> Ah, I see! Thank you. I feel I'm a stupid... I'll do so in v2.
> 

Not at all.

It's easy for me to not introduce bugs because I never write new code.
I just sit here all day obsessing about error handling.

regards,
dan carpenter
  
Mauro Carvalho Chehab Nov. 8, 2021, 4:35 p.m. UTC | #6
Em Tue, 09 Nov 2021 00:25:52 +0900
Tsuchiya Yuto <kitakar@gmail.com> escreveu:

> <removed Alan from Cc as the mail address not reachable>
> 
> On Mon, 2021-11-08 at 18:14 +0300, Dan Carpenter wrote:
> > On Tue, Nov 09, 2021 at 12:00:29AM +0900, Tsuchiya Yuto wrote:  
> > > On Tue, 2021-11-02 at 14:33 +0300, Dan Carpenter wrote:  
> > > > On Mon, Oct 18, 2021 at 01:19:45AM +0900, Tsuchiya Yuto wrote:  
> > > > > The function ia_css_mipi_is_source_port_valid() returns true if the port
> > > > > is valid. So, we can't use the existing err variable as is.
> > > > > 
> > > > > To fix this issue while reusing that variable, invert the return value
> > > > > when assigning it to the variable.
> > > > > 
> > > > > Fixes: 3c0538fbad9f ("media: atomisp: get rid of most checks for ISP2401 version")
> > > > > Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> > > > > ---
> > > > >  .../staging/media/atomisp/pci/sh_css_mipi.c   | 24 ++++++++++++-------
> > > > >  1 file changed, 15 insertions(+), 9 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > > > index 65fc93c5d56b..c1f2f6151c5f 100644
> > > > > --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > > > +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > > > @@ -423,10 +423,12 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
> > > > >  		return 0; /* AM TODO: Check  */
> > > > >  	}
> > > > >  
> > > > > -	if (!IS_ISP2401)
> > > > > +	if (!IS_ISP2401) {
> > > > >  		port = (unsigned int)pipe->stream->config.source.port.port;
> > > > > -	else
> > > > > -		err = ia_css_mipi_is_source_port_valid(pipe, &port);
> > > > > +	} else {
> > > > > +		/* Returns true if port is valid. So, invert it */
> > > > > +		err = !ia_css_mipi_is_source_port_valid(pipe, &port);  
> > > > 
> > > > Don't invert it...  This isn't supposed to return 1 on failure it's
> > > > supposed to return negative error codes.  
> > > 
> > > You mean I should instead modify the return value of
> > > ia_css_mipi_is_source_port_valid() ?
> > >   
> > 
> > No.  ia_css_mipi_is_source_port_valid() is fine.  It has a boolean name
> > so returning bool is fine.  What I'm saying is that allocate_mipi_frames()
> > should do:
> > 
> > 	if (!ia_css_mipi_is_source_port_valid(pipe, &port))
> > 		err = -EINVAL;
> > 
> > Otherwise it returns negative error codes and 1 on failure.  
> 
> Ah, I see! Thank you. I feel I'm a stupid... I'll do so in v2.

I would prefer if you could send such changes on new patches.

Regards,
Mauro
  
Tsuchiya Yuto Dec. 1, 2021, 12:15 p.m. UTC | #7
On Mon, 2021-11-08 at 16:35 +0000, Mauro Carvalho Chehab wrote:
> Em Tue, 09 Nov 2021 00:25:52 +0900
> Tsuchiya Yuto <kitakar@gmail.com> escreveu:
> 
> > <removed Alan from Cc as the mail address not reachable>
> > 
> > On Mon, 2021-11-08 at 18:14 +0300, Dan Carpenter wrote:
> > > On Tue, Nov 09, 2021 at 12:00:29AM +0900, Tsuchiya Yuto wrote:  
> > > > On Tue, 2021-11-02 at 14:33 +0300, Dan Carpenter wrote:  
> > > > > On Mon, Oct 18, 2021 at 01:19:45AM +0900, Tsuchiya Yuto wrote:  
> > > > > > The function ia_css_mipi_is_source_port_valid() returns true if the port
> > > > > > is valid. So, we can't use the existing err variable as is.
> > > > > > 
> > > > > > To fix this issue while reusing that variable, invert the return value
> > > > > > when assigning it to the variable.
> > > > > > 
> > > > > > Fixes: 3c0538fbad9f ("media: atomisp: get rid of most checks for ISP2401 version")
> > > > > > Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> > > > > > ---
> > > > > >  .../staging/media/atomisp/pci/sh_css_mipi.c   | 24 ++++++++++++-------
> > > > > >  1 file changed, 15 insertions(+), 9 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > > > > index 65fc93c5d56b..c1f2f6151c5f 100644
> > > > > > --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > > > > +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > > > > @@ -423,10 +423,12 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
> > > > > >  		return 0; /* AM TODO: Check  */
> > > > > >  	}
> > > > > >  
> > > > > > -	if (!IS_ISP2401)
> > > > > > +	if (!IS_ISP2401) {
> > > > > >  		port = (unsigned int)pipe->stream->config.source.port.port;
> > > > > > -	else
> > > > > > -		err = ia_css_mipi_is_source_port_valid(pipe, &port);
> > > > > > +	} else {
> > > > > > +		/* Returns true if port is valid. So, invert it */
> > > > > > +		err = !ia_css_mipi_is_source_port_valid(pipe, &port);  
> > > > > 
> > > > > Don't invert it...  This isn't supposed to return 1 on failure it's
> > > > > supposed to return negative error codes.  
> > > > 
> > > > You mean I should instead modify the return value of
> > > > ia_css_mipi_is_source_port_valid() ?
> > > >   
> > > 
> > > No.  ia_css_mipi_is_source_port_valid() is fine.  It has a boolean name
> > > so returning bool is fine.  What I'm saying is that allocate_mipi_frames()
> > > should do:
> > > 
> > > 	if (!ia_css_mipi_is_source_port_valid(pipe, &port))
> > > 		err = -EINVAL;
> > > 
> > > Otherwise it returns negative error codes and 1 on failure.  
> > 
> > Ah, I see! Thank you. I feel I'm a stupid... I'll do so in v2.
> 
> I would prefer if you could send such changes on new patches.

I'm a little bit too late, sorry. For the record, the return value issue
pointed out here is already gone with patch ("media: atomisp: sh_css_mipi:
cleanup the code") [1]. Thanks!

[1] https://lore.kernel.org/linux-media/b541d4c9923154be7ae0518d01ce994acbef3f9b.1637142905.git.mchehab+huawei@kernel.org/
  

Patch

diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
index 65fc93c5d56b..c1f2f6151c5f 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
@@ -423,10 +423,12 @@  allocate_mipi_frames(struct ia_css_pipe *pipe,
 		return 0; /* AM TODO: Check  */
 	}
 
-	if (!IS_ISP2401)
+	if (!IS_ISP2401) {
 		port = (unsigned int)pipe->stream->config.source.port.port;
-	else
-		err = ia_css_mipi_is_source_port_valid(pipe, &port);
+	} else {
+		/* Returns true if port is valid. So, invert it */
+		err = !ia_css_mipi_is_source_port_valid(pipe, &port);
+	}
 
 	assert(port < N_CSI_PORTS);
 
@@ -553,10 +555,12 @@  free_mipi_frames(struct ia_css_pipe *pipe)
 			return err;
 		}
 
-		if (!IS_ISP2401)
+		if (!IS_ISP2401) {
 			port = (unsigned int)pipe->stream->config.source.port.port;
-		else
-			err = ia_css_mipi_is_source_port_valid(pipe, &port);
+		} else {
+			/* Returns true if port is valid. So, invert it */
+			err = !ia_css_mipi_is_source_port_valid(pipe, &port);
+		}
 
 		assert(port < N_CSI_PORTS);
 
@@ -665,10 +669,12 @@  send_mipi_frames(struct ia_css_pipe *pipe)
 		/* TODO: AM: maybe this should be returning an error. */
 	}
 
-	if (!IS_ISP2401)
+	if (!IS_ISP2401) {
 		port = (unsigned int)pipe->stream->config.source.port.port;
-	else
-		err = ia_css_mipi_is_source_port_valid(pipe, &port);
+	} else {
+		/* Returns true if port is valid. So, invert it */
+		err = !ia_css_mipi_is_source_port_valid(pipe, &port);
+	}
 
 	assert(port < N_CSI_PORTS);