[PATCH] usb: typec: ucsi_ccg: use dev_info for timeout error

[PATCH] usb: typec: ucsi_ccg: use dev_info for timeout error

[Ajay Gupta]

From: Ajay Gupta <ajayg@nvidia.com>

Timeout error may be seen due to missing USB Type-C UCSI controller
on some NVIDIA GPU card. Currently we don't have a correct way to
identify these cards.

Tools like Plymouth (splashscreen) doesn't like dev_err so
changing timeout status log to dev_info.

Bug information:
https://bugzilla.kernel.org/show_bug.cgi?id=206653

Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
---
 drivers/usb/typec/ucsi/ucsi_ccg.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index bff96d64dddf..d69432df866e 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -252,7 +252,7 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
 		put_unaligned_le16(rab, buf);
 		status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
 		if (status < 0) {
-			dev_err(uc->dev, "i2c_transfer failed %d\n", status);
+			dev_info(uc->dev, "i2c_transfer failed %d\n", status);
 			pm_runtime_put_sync(uc->dev);
 			return status;
 		}
@@ -289,7 +289,7 @@ static int ccg_write(struct ucsi_ccg *uc, u16 rab, const u8 *data, u32 len)
 	pm_runtime_get_sync(uc->dev);
 	status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
 	if (status < 0) {
-		dev_err(uc->dev, "i2c_transfer failed %d\n", status);
+		dev_info(uc->dev, "i2c_transfer failed %d\n", status);
 		pm_runtime_put_sync(uc->dev);
 		kfree(buf);
 		return status;
@@ -1345,7 +1345,7 @@ static int ucsi_ccg_probe(struct i2c_client *client,
 	/* reset ccg device and initialize ucsi */
 	status = ucsi_ccg_init(uc);
 	if (status < 0) {
-		dev_err(uc->dev, "ucsi_ccg_init failed - %d\n", status);
+		dev_info(uc->dev, "ucsi_ccg_init failed - %d\n", status);
 		return status;
 	}
 
-- 
2.17.1

Re: [PATCH] usb: typec: ucsi_ccg: use dev_info for timeout error

[Greg KH]

On Mon, Sep 21, 2020 at 10:42:16AM -0700, Ajay Gupta wrote:
> From: Ajay Gupta <ajayg@nvidia.com>
> 
> Timeout error may be seen due to missing USB Type-C UCSI controller
> on some NVIDIA GPU card. Currently we don't have a correct way to
> identify these cards.

No unique id anywhere???  That feels like someone really messed up :(

> 
> Tools like Plymouth (splashscreen) doesn't like dev_err so
> changing timeout status log to dev_info.
> 
> Bug information:
> https://bugzilla.kernel.org/show_bug.cgi?id=206653
> 
> Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
> ---
>  drivers/usb/typec/ucsi/ucsi_ccg.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
> index bff96d64dddf..d69432df866e 100644
> --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -252,7 +252,7 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
>  		put_unaligned_le16(rab, buf);
>  		status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
>  		if (status < 0) {
> -			dev_err(uc->dev, "i2c_transfer failed %d\n", status);
> +			dev_info(uc->dev, "i2c_transfer failed %d\n", status);

It's an error, leave it an error.

If it's not an error, don't log it.

Or better yet, fix the hardware :)

thanks,

greg k-h

RE: [PATCH] usb: typec: ucsi_ccg: use dev_info for timeout error

[Ajay Gupta]

Hi Greg

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Monday, September 21, 2020 11:55 AM
> To: Ajay Gupta <ajaykuee@gmail.com>
> Cc: heikki.krogerus@linux.intel.com; linux-usb@vger.kernel.org; Ajay Gupta
> <ajayg@nvidia.com>
> Subject: Re: [PATCH] usb: typec: ucsi_ccg: use dev_info for timeout error
> 
> External email: Use caution opening links or attachments
> 
> 
> On Mon, Sep 21, 2020 at 10:42:16AM -0700, Ajay Gupta wrote:
> > From: Ajay Gupta <ajayg@nvidia.com>
> >
> > Timeout error may be seen due to missing USB Type-C UCSI controller on
> > some NVIDIA GPU card. Currently we don't have a correct way to
> > identify these cards.
> 
> No unique id anywhere???  That feels like someone really messed up :(
Yes, there is none.
> 
> >
> > Tools like Plymouth (splashscreen) doesn't like dev_err so changing
> > timeout status log to dev_info.
> >
> > Bug information:
> > https://bugzilla.kernel.org/show_bug.cgi?id=206653
> >
> > Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
> > ---
> >  drivers/usb/typec/ucsi/ucsi_ccg.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > index bff96d64dddf..d69432df866e 100644
> > --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > @@ -252,7 +252,7 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8
> *data, u32 len)
> >               put_unaligned_le16(rab, buf);
> >               status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> >               if (status < 0) {
> > -                     dev_err(uc->dev, "i2c_transfer failed %d\n", status);
> > +                     dev_info(uc->dev, "i2c_transfer failed %d\n",
> > + status);
> 
> It's an error, leave it an error.
> 
> If it's not an error, don't log it.
Looks like this is the best option we have.

Thanks
> nvpublic
> 
> Or better yet, fix the hardware :)
> 
> thanks,
> 
> greg k-h

Re: [PATCH] usb: typec: ucsi_ccg: use dev_info for timeout error

[Greg KH]

On Mon, Sep 21, 2020 at 08:50:04PM +0000, Ajay Gupta wrote:
> Hi Greg
> 
> > -----Original Message-----
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: Monday, September 21, 2020 11:55 AM
> > To: Ajay Gupta <ajaykuee@gmail.com>
> > Cc: heikki.krogerus@linux.intel.com; linux-usb@vger.kernel.org; Ajay Gupta
> > <ajayg@nvidia.com>
> > Subject: Re: [PATCH] usb: typec: ucsi_ccg: use dev_info for timeout error
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Mon, Sep 21, 2020 at 10:42:16AM -0700, Ajay Gupta wrote:
> > > From: Ajay Gupta <ajayg@nvidia.com>
> > >
> > > Timeout error may be seen due to missing USB Type-C UCSI controller on
> > > some NVIDIA GPU card. Currently we don't have a correct way to
> > > identify these cards.
> > 
> > No unique id anywhere???  That feels like someone really messed up :(
> Yes, there is none.

Can you identify them by the failure of an i2c transfer?  Feels like
that would work :)

> > > Tools like Plymouth (splashscreen) doesn't like dev_err so changing
> > > timeout status log to dev_info.
> > >
> > > Bug information:
> > > https://bugzilla.kernel.org/show_bug.cgi?id=206653
> > >
> > > Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
> > > ---
> > >  drivers/usb/typec/ucsi/ucsi_ccg.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > > b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > > index bff96d64dddf..d69432df866e 100644
> > > --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > > +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > > @@ -252,7 +252,7 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8
> > *data, u32 len)
> > >               put_unaligned_le16(rab, buf);
> > >               status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > >               if (status < 0) {
> > > -                     dev_err(uc->dev, "i2c_transfer failed %d\n", status);
> > > +                     dev_info(uc->dev, "i2c_transfer failed %d\n",
> > > + status);
> > 
> > It's an error, leave it an error.
> > 
> > If it's not an error, don't log it.
> Looks like this is the best option we have.

It is an error, so we log it, that's fine.  We are correctly reporting
the fact that the hardware is broken and not working properly, don't
paper over that please.

thanks,

greg k-h

RE: [PATCH] usb: typec: ucsi_ccg: use dev_info for timeout error

[Ajay Gupta]

Hi Greg

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Monday, September 21, 2020 10:44 PM
> To: Ajay Gupta <ajayg@nvidia.com>
> Cc: Ajay Gupta <ajaykuee@gmail.com>; heikki.krogerus@linux.intel.com;
> linux-usb@vger.kernel.org
> Subject: Re: [PATCH] usb: typec: ucsi_ccg: use dev_info for timeout error
> 
> External email: Use caution opening links or attachments
> 
> 
> On Mon, Sep 21, 2020 at 08:50:04PM +0000, Ajay Gupta wrote:
> > Hi Greg
> >
> > > -----Original Message-----
> > > From: Greg KH <gregkh@linuxfoundation.org>
> > > Sent: Monday, September 21, 2020 11:55 AM
> > > To: Ajay Gupta <ajaykuee@gmail.com>
> > > Cc: heikki.krogerus@linux.intel.com; linux-usb@vger.kernel.org; Ajay
> > > Gupta <ajayg@nvidia.com>
> > > Subject: Re: [PATCH] usb: typec: ucsi_ccg: use dev_info for timeout
> > > error
> > >
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > On Mon, Sep 21, 2020 at 10:42:16AM -0700, Ajay Gupta wrote:
> > > > From: Ajay Gupta <ajayg@nvidia.com>
> > > >
> > > > Timeout error may be seen due to missing USB Type-C UCSI
> > > > controller on some NVIDIA GPU card. Currently we don't have a
> > > > correct way to identify these cards.
> > >
> > > No unique id anywhere???  That feels like someone really messed up
> > > :(
> > Yes, there is none.
> 
> Can you identify them by the failure of an i2c transfer?  Feels like that would
> work :)
Let me test it out and comeback.

> 
> > > > Tools like Plymouth (splashscreen) doesn't like dev_err so
> > > > changing timeout status log to dev_info.
> > > >
> > > > Bug information:
> > > > https://bugzilla.kernel.org/show_bug.cgi?id=206653
> > > >
> > > > Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
> > > > ---
> > > >  drivers/usb/typec/ucsi/ucsi_ccg.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > > > b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > > > index bff96d64dddf..d69432df866e 100644
> > > > --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > > > +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > > > @@ -252,7 +252,7 @@ static int ccg_read(struct ucsi_ccg *uc, u16
> > > > rab, u8
> > > *data, u32 len)
> > > >               put_unaligned_le16(rab, buf);
> > > >               status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > > >               if (status < 0) {
> > > > -                     dev_err(uc->dev, "i2c_transfer failed %d\n", status);
> > > > +                     dev_info(uc->dev, "i2c_transfer failed
> > > > + %d\n", status);
> > >
> > > It's an error, leave it an error.
> > >
> > > If it's not an error, don't log it.
> > Looks like this is the best option we have.
> 
> It is an error, so we log it, that's fine.  We are correctly reporting the fact that
> the hardware is broken and not working properly, don't paper over that
> please.
Ok. 

Thanks
>nvpublic
> 
> thanks,
> 
> greg k-h