Re: [PATCH 3/3] staging: typec: Fix one defect of incorrect type assignment.

Re: [PATCH 3/3] staging: typec: Fix one defect of incorrect type assignment.

[Dan Carpenter]

On Thu, Jun 01, 2017 at 07:21:38AM +0000, ? ? wrote:
> From: Pan Li <incarnation.p.lee@outlook.com>
> 
>     Convert type cpu to le16 for consistent type assignment.
> 
> Signed-off-by: Pan Li <incarnation.p.lee@outlook.com>
> ---
>  drivers/staging/typec/tcpci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index 18d2e06..dac41f2 100644
> --- a/drivers/staging/typec/tcpci.c
> +++ b/drivers/staging/typec/tcpci.c
> @@ -390,7 +390,7 @@ static irqreturn_t tcpci_irq(int irq, void *dev_id)
>  		regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
>  
>  		tcpci_read16(tcpci, TCPC_RX_HDR, &reg);
> -		msg.header = reg;
> +		msg.header = cpu_to_le16(reg);

This looks like you introduced a bug.  We surely read le values?
Please be more careful.

regards,
dan carpenter

Re: [PATCH 1/3] staging: typec: Fix one defect of incorrect type in argument.

[Dan Carpenter]

On Thu, Jun 01, 2017 at 07:21:00AM +0000, ? ? wrote:
> From: Pan Li <incarnation.p.lee@outlook.com>
> 
>     Convert type le16 to cpu of argument 1 in function pd_header_cnt.
> 
> Signed-off-by: Pan Li <incarnation.p.lee@outlook.com>
> ---
>  drivers/staging/typec/tcpci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index df72d8b..405c9cc 100644
> --- a/drivers/staging/typec/tcpci.c
> +++ b/drivers/staging/typec/tcpci.c
> @@ -287,7 +287,7 @@ static int tcpci_pd_transmit(struct tcpc_dev *tcpc,
>  	unsigned int reg, cnt, header;
>  	int ret;
>  
> -	cnt = msg ? pd_header_cnt(msg->header) * 4 : 0;
> +	cnt = msg ? pd_header_cnt(le16_to_cpu(msg->header)) * 4 : 0;

I guess this one is probably correct...

regards,
dan carpenter

Re: [PATCH 1/3] staging: typec: Fix one defect of incorrect type in argument.

[Guenter Roeck]

On Thu, Jun 01, 2017 at 05:57:14PM +0300, Dan Carpenter wrote:
> On Thu, Jun 01, 2017 at 07:21:00AM +0000, ? ? wrote:
> > From: Pan Li <incarnation.p.lee@outlook.com>
> > 
> >     Convert type le16 to cpu of argument 1 in function pd_header_cnt.
> > 
> > Signed-off-by: Pan Li <incarnation.p.lee@outlook.com>
> > ---
> >  drivers/staging/typec/tcpci.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> > index df72d8b..405c9cc 100644
> > --- a/drivers/staging/typec/tcpci.c
> > +++ b/drivers/staging/typec/tcpci.c
> > @@ -287,7 +287,7 @@ static int tcpci_pd_transmit(struct tcpc_dev *tcpc,
> >  	unsigned int reg, cnt, header;
> >  	int ret;
> >  
> > -	cnt = msg ? pd_header_cnt(msg->header) * 4 : 0;
> > +	cnt = msg ? pd_header_cnt(le16_to_cpu(msg->header)) * 4 : 0;
> 
> I guess this one is probably correct...
> 
Yes. Overall though the series only touches the surface; the driver as-is is
just not endianness clean. Someone will have to go through it and fix all
problems. For example, tcpci_read16() is flawed since it reads 16 bits into
a unrestricted 32 bit variable pointer. This needs some active work and can
not rely on code analysis to find and fix the all flaws. Something to add
to TODO.

Side note: The subject lines in this series are all messed up.

Thanks,
Guenter

Re: [PATCH 2/3] staging: typec: Fix one defect of incorrect type assignment.

[Dan Carpenter]

On Thu, Jun 01, 2017 at 07:21:22AM +0000, ? ? wrote:
> From: Pan Li <incarnation.p.lee@outlook.com>
> 
>     Convert type le16 to cpu for consistent type assignment.
> 
> Signed-off-by: Pan Li <incarnation.p.lee@outlook.com>
> ---
>  drivers/staging/typec/tcpci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index 405c9cc..18d2e06 100644
> --- a/drivers/staging/typec/tcpci.c
> +++ b/drivers/staging/typec/tcpci.c
> @@ -292,7 +292,7 @@ static int tcpci_pd_transmit(struct tcpc_dev *tcpc,
>  	if (ret < 0)
>  		return ret;
>  
> -	header = msg ? msg->header : 0;
> +	header = msg ? le16_to_cpu(msg->header) : 0;
>  	ret = tcpci_write16(tcpci, TCPC_TX_HDR, header);

This looks like it introduces bugs.  We normally write endian data and
you've changed it to write CPU endian data.

regards,
dan carpenter

Re: [PATCH 0/3] staging:typec: Fix 3 defect of incorrect type

[Guenter Roeck]

On 06/01/2017 12:20 AM, ? ? wrote:
> From: Pan Li <incarnation.p.lee@outlook.com>
> 
> Pan Li (3):
>    Fix one defect of incorrect type in argument.
>    Fix one defect of incorrect type assignment.
>    Fix one defect of incorrect type assignment.
> 
>   drivers/staging/typec/tcpci.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 

Please merge those patches into one. They all fix endianness issues.

Thanks,
Guenter