Silence following sparse warning:
drivers/staging/comedi/drivers/dt2814.c: In function ‘dt2814_interrupt’:
drivers/staging/comedi/drivers/dt2814.c:193:6: warning: variable ‘data’ set but not used [-Wunused-but-set-variable]
int data;
^~~~
drivers/staging/comedi/drivers/dt2814.c: In function ‘dt2814_attach’:
drivers/staging/comedi/drivers/dt2814.c:232:6: warning: variable ‘i’ set but not used [-Wunused-but-set-variable]
int i;
^
Signed-off-by: Gaurav Pathak <[email protected]>
---
drivers/staging/comedi/drivers/dt2814.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/drivers/staging/comedi/drivers/dt2814.c b/drivers/staging/comedi/drivers/dt2814.c
index d2c715737361..eea587d63e18 100644
--- a/drivers/staging/comedi/drivers/dt2814.c
+++ b/drivers/staging/comedi/drivers/dt2814.c
@@ -186,22 +186,15 @@ static int dt2814_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
static irqreturn_t dt2814_interrupt(int irq, void *d)
{
- int lo, hi;
struct comedi_device *dev = d;
struct dt2814_private *devpriv = dev->private;
struct comedi_subdevice *s = dev->read_subdev;
- int data;
if (!dev->attached) {
dev_err(dev->class_dev, "spurious interrupt\n");
return IRQ_HANDLED;
}
- hi = inb(dev->iobase + DT2814_DATA);
- lo = inb(dev->iobase + DT2814_DATA);
-
- data = (hi << 4) | (lo >> 4);
-
if (!(--devpriv->ntrig)) {
int i;
@@ -229,7 +222,6 @@ static int dt2814_attach(struct comedi_device *dev, struct comedi_devconfig *it)
struct dt2814_private *devpriv;
struct comedi_subdevice *s;
int ret;
- int i;
ret = comedi_request_region(dev, it->options[0], 0x2);
if (ret)
@@ -241,8 +233,6 @@ static int dt2814_attach(struct comedi_device *dev, struct comedi_devconfig *it)
dev_err(dev->class_dev, "reset error (fatal)\n");
return -EIO;
}
- i = inb(dev->iobase + DT2814_DATA);
- i = inb(dev->iobase + DT2814_DATA);
if (it->options[1]) {
ret = request_irq(it->options[1], dt2814_interrupt, 0,
--
2.17.1
Hi,
On Sun, May 24, 2020 at 05:06:18PM +0530, Gaurav Pathak wrote:
> Silence following sparse warning:
> drivers/staging/comedi/drivers/dt2814.c: In function ‘dt2814_interrupt’:
> drivers/staging/comedi/drivers/dt2814.c:193:6: warning: variable ‘data’ set but not used [-Wunused-but-set-variable]
> int data;
> ^~~~
> drivers/staging/comedi/drivers/dt2814.c: In function ‘dt2814_attach’:
> drivers/staging/comedi/drivers/dt2814.c:232:6: warning: variable ‘i’ set but not used [-Wunused-but-set-variable]
> int i;
> ^
These warnings are not from sparse but simply from the compiler.
> static irqreturn_t dt2814_interrupt(int irq, void *d)
> {
> - int lo, hi;
> struct comedi_device *dev = d;
> struct dt2814_private *devpriv = dev->private;
> struct comedi_subdevice *s = dev->read_subdev;
> - int data;
>
> if (!dev->attached) {
> dev_err(dev->class_dev, "spurious interrupt\n");
> return IRQ_HANDLED;
> }
>
> - hi = inb(dev->iobase + DT2814_DATA);
> - lo = inb(dev->iobase + DT2814_DATA);
> -
> - data = (hi << 4) | (lo >> 4);
OK, 'data' is unused but are these 2 'inb(dev->iobase + DT2814_DATA)'
needed or not? I would guess that they're needed but I don't know
this hardware.
> @@ -241,8 +233,6 @@ static int dt2814_attach(struct comedi_device *dev, struct comedi_devconfig *it)
> dev_err(dev->class_dev, "reset error (fatal)\n");
> return -EIO;
> }
> - i = inb(dev->iobase + DT2814_DATA);
> - i = inb(dev->iobase + DT2814_DATA);
Same here.
-- Luc
Thanks a lot for your reply.
I am trying to run sparse on drivers/staging directory,
but I am not getting any useful warnings.
Steps that I did:
- Referred https://www.kernel.org/doc/man-pages/linux-next.html to get
linux-next latest tag next-20200522.
- After that executed following commands:
- make mrproper; make clean; make distclean
- make defconfig
- make menuconfig (Enabled all Staging Drivers with M)
- make modules_prepare
- make C=1 M=drivers/staging (Also tried individual directories
inside staging directory e.g. make C=1 M=drivers/staging/comedi/drivers)
I am not getting any warning, I have attached the output in text format.
Any pointers for finding warning using sparse in drivers/staging
directory will be helpful. I really appretiate your guidance in this
regard.
On Sun, May 24, 2020 at 02:10:44PM +0200, Luc Van Oostenryck wrote:
> Hi,
>
> On Sun, May 24, 2020 at 05:06:18PM +0530, Gaurav Pathak wrote:
> > Silence following sparse warning:
> > drivers/staging/comedi/drivers/dt2814.c: In function ‘dt2814_interrupt’:
> > drivers/staging/comedi/drivers/dt2814.c:193:6: warning: variable ‘data’ set but not used [-Wunused-but-set-variable]
> > int data;
> > ^~~~
> > drivers/staging/comedi/drivers/dt2814.c: In function ‘dt2814_attach’:
> > drivers/staging/comedi/drivers/dt2814.c:232:6: warning: variable ‘i’ set but not used [-Wunused-but-set-variable]
> > int i;
> > ^
>
> These warnings are not from sparse but simply from the compiler.
>
> > static irqreturn_t dt2814_interrupt(int irq, void *d)
> > {
> > - int lo, hi;
> > struct comedi_device *dev = d;
> > struct dt2814_private *devpriv = dev->private;
> > struct comedi_subdevice *s = dev->read_subdev;
> > - int data;
> >
> > if (!dev->attached) {
> > dev_err(dev->class_dev, "spurious interrupt\n");
> > return IRQ_HANDLED;
> > }
> >
> > - hi = inb(dev->iobase + DT2814_DATA);
> > - lo = inb(dev->iobase + DT2814_DATA);
> > -
> > - data = (hi << 4) | (lo >> 4);
>
> OK, 'data' is unused but are these 2 'inb(dev->iobase + DT2814_DATA)'
> needed or not? I would guess that they're needed but I don't know
> this hardware.
>
> > @@ -241,8 +233,6 @@ static int dt2814_attach(struct comedi_device *dev, struct comedi_devconfig *it)
> > dev_err(dev->class_dev, "reset error (fatal)\n");
> > return -EIO;
> > }
> > - i = inb(dev->iobase + DT2814_DATA);
> > - i = inb(dev->iobase + DT2814_DATA);
>
> Same here.
>
> -- Luc
Nevermind, my sparse version was broken. Installed sparse 0.6.1, but
still no warnings from sparse in any of the directories in drivers staging.
E.g.:
make C=2 M=drivers/staging/sm750fb
CHECK drivers/staging/sm750fb/sm750.c
CHECK drivers/staging/sm750fb/sm750_hw.c
CHECK drivers/staging/sm750fb/sm750_accel.c
CHECK drivers/staging/sm750fb/sm750_cursor.c
CHECK drivers/staging/sm750fb/ddk750_chip.c
CHECK drivers/staging/sm750fb/ddk750_power.c
CHECK drivers/staging/sm750fb/ddk750_mode.c
CHECK drivers/staging/sm750fb/ddk750_display.c
CHECK drivers/staging/sm750fb/ddk750_swi2c.c
CHECK drivers/staging/sm750fb/ddk750_sii164.c
CHECK drivers/staging/sm750fb/ddk750_dvi.c
CHECK drivers/staging/sm750fb/ddk750_hwi2c.c
MODPOST 1 modules
For few drivers, I am getting:
CC [M] drivers/staging/vt6656/key.o
CHECK drivers/staging/vt6656/rf.c
/usr/lib/gcc/x86_64-linux-gnu/7/include/stddef.h:417:9: warning: preprocessor token offsetof redefined
./include/linux/stddef.h:17:9: this was the original definition
but I guess these are false positive, as #undef offsetof is already present in ./include/linux/stddef.h.
On Sun, May 24, 2020 at 02:10:44PM +0200, Luc Van Oostenryck wrote:
> Hi,
>
> On Sun, May 24, 2020 at 05:06:18PM +0530, Gaurav Pathak wrote:
> > Silence following sparse warning:
> > drivers/staging/comedi/drivers/dt2814.c: In function ‘dt2814_interrupt’:
> > drivers/staging/comedi/drivers/dt2814.c:193:6: warning: variable ‘data’ set but not used [-Wunused-but-set-variable]
> > int data;
> > ^~~~
> > drivers/staging/comedi/drivers/dt2814.c: In function ‘dt2814_attach’:
> > drivers/staging/comedi/drivers/dt2814.c:232:6: warning: variable ‘i’ set but not used [-Wunused-but-set-variable]
> > int i;
> > ^
>
> These warnings are not from sparse but simply from the compiler.
>
> > static irqreturn_t dt2814_interrupt(int irq, void *d)
> > {
> > - int lo, hi;
> > struct comedi_device *dev = d;
> > struct dt2814_private *devpriv = dev->private;
> > struct comedi_subdevice *s = dev->read_subdev;
> > - int data;
> >
> > if (!dev->attached) {
> > dev_err(dev->class_dev, "spurious interrupt\n");
> > return IRQ_HANDLED;
> > }
> >
> > - hi = inb(dev->iobase + DT2814_DATA);
> > - lo = inb(dev->iobase + DT2814_DATA);
> > -
> > - data = (hi << 4) | (lo >> 4);
>
> OK, 'data' is unused but are these 2 'inb(dev->iobase + DT2814_DATA)'
> needed or not? I would guess that they're needed but I don't know
> this hardware.
>
> > @@ -241,8 +233,6 @@ static int dt2814_attach(struct comedi_device *dev, struct comedi_devconfig *it)
> > dev_err(dev->class_dev, "reset error (fatal)\n");
> > return -EIO;
> > }
> > - i = inb(dev->iobase + DT2814_DATA);
> > - i = inb(dev->iobase + DT2814_DATA);
>
> Same here.
>
> -- Luc
On Sun, May 24, 2020 at 06:19:22PM +0530, GAURAV PATHAK wrote:
> Thanks a lot for your reply.
> I am trying to run sparse on drivers/staging directory,
> but I am not getting any useful warnings.
> Steps that I did:
>
> - Referred https://www.kernel.org/doc/man-pages/linux-next.html to get
> linux-next latest tag next-20200522.
> - After that executed following commands:
> - make mrproper; make clean; make distclean
> - make defconfig
> - make menuconfig (Enabled all Staging Drivers with M)
> - make modules_prepare
> - make C=1 M=drivers/staging (Also tried individual directories
> inside staging directory e.g. make C=1 M=drivers/staging/comedi/drivers)
>
> I am not getting any warning, I have attached the output in text format.
>
> Any pointers for finding warning using sparse in drivers/staging
> directory will be helpful. I really appretiate your guidance in this
> regard.
Well it seems that most staging drivers have no sparse warnings at all.
I only see somes in the following staging drivers:
drivers/staging/isdn/
drivers/staging/kpc2000/
drivers/staging/uwb/
drivers/staging/vc04_services/
drivers/staging/wfx/
drivers/staging/wusbcore/
Your commands seems to be good but I would advice you to use 'C=2'
instead of 'C=1' in order for sparse to effectively run on each
input file even when the corresponding .o file already exists.
The simplest to use is:
- make allmodconfig
- make -j8 drivers/staging/
This will just compile all the files without running sparse.
So all warnings will be from the compiler.
The you can run:
- make C=2 drivers/staging/
which will only run sparse (and will thus be much faster).
Best regards,
-- Luc
On Sun, May 24, 2020 at 07:02:20PM +0530, GAURAV PATHAK wrote:
>
> For few drivers, I am getting:
>
> CC [M] drivers/staging/vt6656/key.o
> CHECK drivers/staging/vt6656/rf.c
> /usr/lib/gcc/x86_64-linux-gnu/7/include/stddef.h:417:9: warning: preprocessor token offsetof redefined
> ./include/linux/stddef.h:17:9: this was the original definition
>
> but I guess these are false positive, as #undef offsetof is already present in ./include/linux/stddef.h.
But there is no #undef in /usr/lib/gcc/x86_64-linux-gnu/7/include/stddef.h
which seems to be sometimes included after Linux's include/linux/stddef.h
-- Luc
Your subject doesn't use the correct patch prefix please use.
[PATCH] Staging: comedi: dt2814: remove unused assignments
Please resend a v2.
Correct the references to Sparse as well like Luc said.
regards,
dan carpenter
On Tue, May 26, 2020 at 05:13:46PM +0300, Dan Carpenter wrote:
> Your subject doesn't use the correct patch prefix please use.
>
> [PATCH] Staging: comedi: dt2814: remove unused assignments
>
> Please resend a v2.
>
> Correct the references to Sparse as well like Luc said.
>
> regards,
> dan carpenter
>
Hello Dan,
Thank you for reviewing and suggesting changes. I have modified the
patch and attached it with the e-mail to keep this thread conversation.
I hope I have made the changes correctly.
Thanks,
Gaurav
On Tue, May 26, 2020 at 08:24:50PM +0530, GAURAV PATHAK wrote:
> On Tue, May 26, 2020 at 05:13:46PM +0300, Dan Carpenter wrote:
> > Your subject doesn't use the correct patch prefix please use.
> >
> > [PATCH] Staging: comedi: dt2814: remove unused assignments
> >
> > Please resend a v2.
> >
> > Correct the references to Sparse as well like Luc said.
> >
> > regards,
> > dan carpenter
> >
>
> Hello Dan,
> Thank you for reviewing and suggesting changes. I have modified the
> patch and attached it with the e-mail to keep this thread conversation.
> I hope I have made the changes correctly.
>
No, this isn't how to send a v2 patch.
https://www.google.com/search?client=firefox-b-d&q=how+to+send+a+v2+patch
Put [PATCH v2] in the subject. Don't send patches as attachments.
Put a comment after the --- cut off line:
Signed-off...
---
v2: Update subject and commit message.
If you want to reply to the thread, that's good. Use --in-reply-to=<Message-id>
But don't worry about it too much. Greg applies or deletes patches as
soon as he sees them so either way he's not going to have the original
thread in his inbox.
regards,
dan carpenter
Silence following compiler warning:
drivers/staging/comedi/drivers/dt2814.c: In function ‘dt2814_interrupt’:
drivers/staging/comedi/drivers/dt2814.c:193:6: warning: variable ‘data’ set but not used [-Wunused-but-set-variable]
int data;
^~~~
drivers/staging/comedi/drivers/dt2814.c: In function ‘dt2814_attach’:
drivers/staging/comedi/drivers/dt2814.c:232:6: warning: variable ‘i’ set but not used [-Wunused-but-set-variable]
int i;
^
Signed-off-by: Gaurav Pathak <[email protected]>
---
v2: Update subject and commit message.
drivers/staging/comedi/drivers/dt2814.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/drivers/staging/comedi/drivers/dt2814.c b/drivers/staging/comedi/drivers/dt2814.c
index d2c715737361..eea587d63e18 100644
--- a/drivers/staging/comedi/drivers/dt2814.c
+++ b/drivers/staging/comedi/drivers/dt2814.c
@@ -186,22 +186,15 @@ static int dt2814_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
static irqreturn_t dt2814_interrupt(int irq, void *d)
{
- int lo, hi;
struct comedi_device *dev = d;
struct dt2814_private *devpriv = dev->private;
struct comedi_subdevice *s = dev->read_subdev;
- int data;
if (!dev->attached) {
dev_err(dev->class_dev, "spurious interrupt\n");
return IRQ_HANDLED;
}
- hi = inb(dev->iobase + DT2814_DATA);
- lo = inb(dev->iobase + DT2814_DATA);
-
- data = (hi << 4) | (lo >> 4);
-
if (!(--devpriv->ntrig)) {
int i;
@@ -229,7 +222,6 @@ static int dt2814_attach(struct comedi_device *dev, struct comedi_devconfig *it)
struct dt2814_private *devpriv;
struct comedi_subdevice *s;
int ret;
- int i;
ret = comedi_request_region(dev, it->options[0], 0x2);
if (ret)
@@ -241,8 +233,6 @@ static int dt2814_attach(struct comedi_device *dev, struct comedi_devconfig *it)
dev_err(dev->class_dev, "reset error (fatal)\n");
return -EIO;
}
- i = inb(dev->iobase + DT2814_DATA);
- i = inb(dev->iobase + DT2814_DATA);
if (it->options[1]) {
ret = request_irq(it->options[1], dt2814_interrupt, 0,
--
2.17.1
On Tue, May 26, 2020 at 09:20:16PM +0530, gaurav wrote:
^^^^^^
So very close except your from header isn't right. Just fix that and
send a v3.
regards,
dan carpenter
From: Gaurav Pathak <[email protected]>
Silence following compiler warning:
drivers/staging/comedi/drivers/dt2814.c: In function ‘dt2814_interrupt’:
drivers/staging/comedi/drivers/dt2814.c:193:6: warning: variable ‘data’ set but not used [-Wunused-but-set-variable]
int data;
^~~~
drivers/staging/comedi/drivers/dt2814.c: In function ‘dt2814_attach’:
drivers/staging/comedi/drivers/dt2814.c:232:6: warning: variable ‘i’ set but not used [-Wunused-but-set-variable]
int i;
^
Signed-off-by: Gaurav Pathak <[email protected]>
---
v3: Update from email header.
drivers/staging/comedi/drivers/dt2814.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/drivers/staging/comedi/drivers/dt2814.c b/drivers/staging/comedi/drivers/dt2814.c
index d2c715737361..eea587d63e18 100644
--- a/drivers/staging/comedi/drivers/dt2814.c
+++ b/drivers/staging/comedi/drivers/dt2814.c
@@ -186,22 +186,15 @@ static int dt2814_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
static irqreturn_t dt2814_interrupt(int irq, void *d)
{
- int lo, hi;
struct comedi_device *dev = d;
struct dt2814_private *devpriv = dev->private;
struct comedi_subdevice *s = dev->read_subdev;
- int data;
if (!dev->attached) {
dev_err(dev->class_dev, "spurious interrupt\n");
return IRQ_HANDLED;
}
- hi = inb(dev->iobase + DT2814_DATA);
- lo = inb(dev->iobase + DT2814_DATA);
-
- data = (hi << 4) | (lo >> 4);
-
if (!(--devpriv->ntrig)) {
int i;
@@ -229,7 +222,6 @@ static int dt2814_attach(struct comedi_device *dev, struct comedi_devconfig *it)
struct dt2814_private *devpriv;
struct comedi_subdevice *s;
int ret;
- int i;
ret = comedi_request_region(dev, it->options[0], 0x2);
if (ret)
@@ -241,8 +233,6 @@ static int dt2814_attach(struct comedi_device *dev, struct comedi_devconfig *it)
dev_err(dev->class_dev, "reset error (fatal)\n");
return -EIO;
}
- i = inb(dev->iobase + DT2814_DATA);
- i = inb(dev->iobase + DT2814_DATA);
if (it->options[1]) {
ret = request_irq(it->options[1], dt2814_interrupt, 0,
--
2.17.1
Oh, crap. I'm really sorry but these lines of code can't be deleted.
Luc was suggesting that earlier and I saw what he said but I was sure
they could be deleted. Now that I look at it more closely I see I was
wrong.
hi = inb(dev->iobase + DT2814_DATA);
lo = inb(dev->iobase + DT2814_DATA);
Every time we read from DT2814_DATA we get different data. The first
time we get the byte and the second time we get the low byte. In other
words reading from the register has side effects so if we delete a read
then the next thing to read from there will get the data that we were
supposed to read.
What we could do instead would be to remove the "hi = " assignment.
There is one other places where the assignment is not used.
i = inb(dev->iobase + DT2814_DATA);
i = inb(dev->iobase + DT2814_DATA);
I feel really bad for not seeing this earlier. I know everyone hates
redoing patches. I certain hate redoing patches. This one was my
fault. :/ The v3 was in the right format and all, but it will cause a
bug.
regards,
dan carpenter