diff mbox

[3/3] tpm_crb: use __le64 annotated variable for response buffer address

Message ID 20180304121205.16934-3-tomas.winkler@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Winkler, Tomas March 4, 2018, 12:12 p.m. UTC
This suppresses sparse warning
drivers/char/tpm/tpm_crb.c:558:18: warning: cast to restricted __le64

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/char/tpm/tpm_crb.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Jarkko Sakkinen March 5, 2018, 1:03 p.m. UTC | #1
On Sun, Mar 04, 2018 at 02:12:05PM +0200, Tomas Winkler wrote:
> This suppresses sparse warning
> drivers/char/tpm/tpm_crb.c:558:18: warning: cast to restricted __le64
> 
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  drivers/char/tpm/tpm_crb.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

The guideline is that you should describe what is wrong rather than
copy-paste the sparse message.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarkko Sakkinen March 6, 2018, 8:28 a.m. UTC | #2
On Mon, Mar 05, 2018 at 03:03:20PM +0200, Jarkko Sakkinen wrote:
> On Sun, Mar 04, 2018 at 02:12:05PM +0200, Tomas Winkler wrote:
> > This suppresses sparse warning
> > drivers/char/tpm/tpm_crb.c:558:18: warning: cast to restricted __le64
> > 
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> >  drivers/char/tpm/tpm_crb.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> The guideline is that you should describe what is wrong rather than
> copy-paste the sparse message.

Jason, didn't yo give the feedback to some patch 1-2 years ago that
instead of copy-pasting parse error one should write a clear commit
msg or is this OK?

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Winkler, Tomas March 6, 2018, 8:34 a.m. UTC | #3
> On Mon, Mar 05, 2018 at 03:03:20PM +0200, Jarkko Sakkinen wrote:
> > On Sun, Mar 04, 2018 at 02:12:05PM +0200, Tomas Winkler wrote:
> > > This suppresses sparse warning
> > > drivers/char/tpm/tpm_crb.c:558:18: warning: cast to restricted
> > > __le64
> > >
> > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > ---
> > >  drivers/char/tpm/tpm_crb.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > The guideline is that you should describe what is wrong rather than
> > copy-paste the sparse message.
> 
> Jason, didn't yo give the feedback to some patch 1-2 years ago that instead
> of copy-pasting parse error one should write a clear commit msg or is this
> OK?

I think you are reading wrongly the rule,  the title explains the issue and in addition 
I'm adding exact sparse warning. this is usually required. 
What is wrong is putting something like 'Fix sparse error' or "Fix warning' into patch subject. 
So the imperative here is 'adding annotation' and not a 'fixing a sparse message'.

Thanks
Tomas

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe March 6, 2018, 3:39 p.m. UTC | #4
On Tue, Mar 06, 2018 at 10:28:21AM +0200, Jarkko Sakkinen wrote:
> On Mon, Mar 05, 2018 at 03:03:20PM +0200, Jarkko Sakkinen wrote:
> > On Sun, Mar 04, 2018 at 02:12:05PM +0200, Tomas Winkler wrote:
> > > This suppresses sparse warning
> > > drivers/char/tpm/tpm_crb.c:558:18: warning: cast to restricted __le64
> > > 
> > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > >  drivers/char/tpm/tpm_crb.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > The guideline is that you should describe what is wrong rather than
> > copy-paste the sparse message.
> 
> Jason, didn't yo give the feedback to some patch 1-2 years ago that
> instead of copy-pasting parse error one should write a clear commit
> msg or is this OK?

The standard is to give some explaination why the tool complaint is
valid and then if suitable include the tool complaint itself.

Eg bad:

Fix sparse warning on resp

Good:

use __le64 annotated variable for response buffer address

IMHO, the subject line sufficiently describes the patch, and it is
generally OK to clip the tool warning into the body..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index acfcdc6f31af..29fecdea0b2d 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -489,6 +489,7 @@  static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	u32 pa_high, pa_low;
 	u64 cmd_pa;
 	u32 cmd_size;
+	__le64 __rsp_pa;
 	u64 rsp_pa;
 	u32 rsp_size;
 	int ret;
@@ -554,8 +555,8 @@  static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 		goto out;
 	}
 
-	memcpy_fromio(&rsp_pa, &priv->regs_t->ctrl_rsp_pa, 8);
-	rsp_pa = le64_to_cpu(rsp_pa);
+	memcpy_fromio(&__rsp_pa, &priv->regs_t->ctrl_rsp_pa, 8);
+	rsp_pa = le64_to_cpu(__rsp_pa);
 	rsp_size = crb_fixup_cmd_size(dev, &io_res, rsp_pa,
 				      ioread32(&priv->regs_t->ctrl_rsp_size));