| Message ID | 1426037325-8392-1-git-send-email-lstoakes@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 11, 2015 at 01:28:40AM +0000, Lorenzo Stoakes wrote: > This patch uses memset_io instead of memset when using memset on __iomem > qualified pointers. This fixes the following sparse warnings:- > > drivers/staging/sm750fb/sm750.c:489:17: warning: incorrect type in argument 1 (different address spaces) > drivers/staging/sm750fb/sm750.c:490:17: warning: incorrect type in argument 1 (different address spaces) > drivers/staging/sm750fb/sm750.c:501:17: warning: incorrect type in argument 1 (different address spaces) > drivers/staging/sm750fb/sm750.c:502:17: warning: incorrect type in argument 1 (different address spaces) > drivers/staging/sm750fb/sm750.c:833:5: warning: incorrect type in argument 1 (different address spaces) > drivers/staging/sm750fb/sm750.c:1154:9: warning: incorrect type in argument 1 (different address spaces) > > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> When I see a patch like this, then I worry, "What if the Sparse annotations are wrong? The patch description doesn't say anything about that." After review then I think the annotations are correct so that's fine. Btw, do you have this hardware? Are you able to test these changes? regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 March 2015 at 08:54, Dan Carpenter <dan.carpenter@oracle.com> wrote: > When I see a patch like this, then I worry, "What if the Sparse > annotations are wrong? The patch description doesn't say anything about > that." After review then I think the annotations are correct so that's > fine. How do you mean? I was careful to check what sparse was referring to, then investigate how memset should be used with pointers with a __iomem qualifier. I'd like to be able to improve my patch descriptions going forward as best I can :) > Btw, do you have this hardware? Are you able to test these changes? Unfortunately not, I am trying to keep these changes as simple code fixes that ought not to affect actual hardware behaviour as I can (though of course you can never be entirely sure that's the case!) I suspect that Sudip must have some real hardware, is this the case Sudip? If it isn't too presumptuous of me to ask, perhaps you might be able to check patches that are successfully merged into staging-testing? Best,
On Wed, Mar 11, 2015 at 09:11:52AM +0000, Lorenzo Stoakes wrote: > On 11 March 2015 at 08:54, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > When I see a patch like this, then I worry, "What if the Sparse > > annotations are wrong? The patch description doesn't say anything about > > that." After review then I think the annotations are correct so that's > > fine. > > How do you mean? I was careful to check what sparse was referring to, > then investigate how memset should be used with pointers with a > __iomem qualifier. I'd like to be able to improve my patch > descriptions going forward as best I can :) > Yes. The patch is correct. I wasn't asking you to redo it. From later patches it's actually clear that you know that this change is a bugfix and a behavior change. But we get a lot of patches where people just randomly change things to please Sparse and it maybe silences a warning but it's not correct. I can think of a few recentish examples where people used standard struct types which hold __iomem or __user pointers but they used them in non-standard ways so the pointers were actually normal kernel pointers. I guess the rule here is that the patch should explain the effect of the bugfix for the user. Often you won't know the effect, but it's a helpful thing to think about. > > Btw, do you have this hardware? Are you able to test these changes? > > Unfortunately not, I am trying to keep these changes as simple code > fixes that ought not to affect actual hardware behaviour as I can > (though of course you can never be entirely sure that's the case!) That's fine. I was just wondering. It affects how paranoid I am when I review the code. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 11, 2015 at 09:11:52AM +0000, Lorenzo Stoakes wrote: > On 11 March 2015 at 08:54, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > Btw, do you have this hardware? Are you able to test these changes? > > Unfortunately not, I am trying to keep these changes as simple code > fixes that ought not to affect actual hardware behaviour as I can > (though of course you can never be entirely sure that's the case!) > > I suspect that Sudip must have some real hardware, is this the case > Sudip? If it isn't too presumptuous of me to ask, perhaps you might be > able to check patches that are successfully merged into > staging-testing? yes, i have the hardware and will test on it. but your patch 5/6 and 6/6 is scaring me :) regards sudip > > Best, > > -- > Lorenzo Stoakes > https:/ljs.io -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 11, 2015 at 03:18:06PM +0530, Sudip Mukherjee wrote: > On Wed, Mar 11, 2015 at 09:11:52AM +0000, Lorenzo Stoakes wrote: > > On 11 March 2015 at 08:54, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > Btw, do you have this hardware? Are you able to test these changes? > > > > Unfortunately not, I am trying to keep these changes as simple code > > fixes that ought not to affect actual hardware behaviour as I can > > (though of course you can never be entirely sure that's the case!) > > > > I suspect that Sudip must have some real hardware, is this the case > > Sudip? If it isn't too presumptuous of me to ask, perhaps you might be > > able to check patches that are successfully merged into > > staging-testing? > yes, i have the hardware and will test on it. but your patch 5/6 and > 6/6 is scaring me :) i think i will better check v2 of your series on hardware, and while you are preparing that v2 keep in mind the changelog should not exceed 72 characters. in your this series for all patches it was more than that. regards sudip > > regards > sudip > > > > Best, > > > > -- > > Lorenzo Stoakes > > https:/ljs.io -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 March 2015 at 10:35, Sudip Mukherjee > i think i will better check v2 of your series on hardware This is incoming in just a moment (though I only v2 patches in the series I've changed which I think is the right way to make modifications with a patch series.) > , and while > you are preparing that v2 keep in mind the changelog should not exceed > 72 characters. in your this series for all patches it was more than > that. I will update the messages in the changed patches accordingly, I'm not sure this is worth a resend of all previous patches for however? I do see quite a few patches in the log that exceed this. Additionally, I suspect it would make the patches less readable to wrap sparse warning lines so I think those ought to sit outside of this limit. I am more than happy to change these though if these ought to be kept *strictly* to a 72 character limit throughout? Best,
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c index aa0888c..3e36b6a 100644 --- a/drivers/staging/sm750fb/sm750.c +++ b/drivers/staging/sm750fb/sm750.c @@ -486,8 +486,8 @@ static int lynxfb_resume(struct pci_dev* pdev) par = info->par; crtc = &par->crtc; cursor = &crtc->cursor; - memset(cursor->vstart, 0x0, cursor->size); - memset(crtc->vScreen,0x0,crtc->vidmem_size); + memset_io(cursor->vstart, 0x0, cursor->size); + memset_io(crtc->vScreen,0x0,crtc->vidmem_size); lynxfb_ops_set_par(info); fb_set_suspend(info, 0); } @@ -498,8 +498,8 @@ static int lynxfb_resume(struct pci_dev* pdev) par = info->par; crtc = &par->crtc; cursor = &crtc->cursor; - memset(cursor->vstart, 0x0, cursor->size); - memset(crtc->vScreen,0x0,crtc->vidmem_size); + memset_io(cursor->vstart, 0x0, cursor->size); + memset_io(crtc->vScreen,0x0,crtc->vidmem_size); lynxfb_ops_set_par(info); fb_set_suspend(info, 0); } @@ -830,7 +830,7 @@ static int lynxfb_set_fbinfo(struct fb_info* info,int index) crtc->cursor.share = share; - memset(crtc->cursor.vstart, 0, crtc->cursor.size); + memset_io(crtc->cursor.vstart, 0, crtc->cursor.size); if(!g_hwcursor){ lynxfb_ops.fb_cursor = NULL; crtc->cursor.disable(&crtc->cursor); @@ -1151,7 +1151,7 @@ static int lynxfb_pci_probe(struct pci_dev * pdev, } #endif - memset(share->pvMem,0,share->vidmem_size); + memset_io(share->pvMem,0,share->vidmem_size); pr_info("sm%3x mmio address = %p\n",share->devid,share->pvReg);
This patch uses memset_io instead of memset when using memset on __iomem qualified pointers. This fixes the following sparse warnings:- drivers/staging/sm750fb/sm750.c:489:17: warning: incorrect type in argument 1 (different address spaces) drivers/staging/sm750fb/sm750.c:490:17: warning: incorrect type in argument 1 (different address spaces) drivers/staging/sm750fb/sm750.c:501:17: warning: incorrect type in argument 1 (different address spaces) drivers/staging/sm750fb/sm750.c:502:17: warning: incorrect type in argument 1 (different address spaces) drivers/staging/sm750fb/sm750.c:833:5: warning: incorrect type in argument 1 (different address spaces) drivers/staging/sm750fb/sm750.c:1154:9: warning: incorrect type in argument 1 (different address spaces) Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> --- drivers/staging/sm750fb/sm750.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)