| Message ID | 1426669046-29935-1-git-send-email-lstoakes@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Why is there a RESEND in the subject. On Wed, Mar 18, 2015 at 08:57:22AM +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) > This changelog still sucks. It doesn't describe the effect of this behavior change for the user. It doesn't even make it clear that you are aware that this is a behavior change. 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 18, 2015 at 01:17:17PM +0300, Dan Carpenter wrote: > Why is there a RESEND in the subject. > > On Wed, Mar 18, 2015 at 08:57:22AM +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) > > > > This changelog still sucks. It doesn't describe the effect of this > behavior change for the user. It doesn't even make it clear that you > are aware that this is a behavior change. It doesn't say to me that you have asked yourself if the sparse annotations are correct. Many times they are wrong. We have had this discussion before but you still sent the same exact bad changelog. 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 18 March 2015 at 10:18, Dan Carpenter <dan.carpenter@oracle.com> wrote: >> >> This changelog still sucks. It doesn't describe the effect of this >> behavior change for the user. It doesn't even make it clear that you >> are aware that this is a behavior change. > > It doesn't say to me that you have asked yourself if the sparse > annotations are correct. Many times they are wrong. My understanding, which as a new contributor is of course limited and likely simply wrong in many aspects, is - these memset's are referring to I/O mapped memory, which as far as I can tell is actually the case here, so in order to make it explicit that this is the case and we know it is, we use memset_io. As far as I understand the pointers simply have a modifier applied which marks them as I/O mapped memory for the purposes of sparse checking whether they are used consistently as such and are not treated like they are a normal kernel pointer. In this case the cursor->vstart and crtc->vScreen pointers, looking through the source, explicitly refer to memory which is I/O mapped, and is annotated as __iomem accordingly throughout. I will update the message accordingly, obviously if I'm misunderstanding something let me know. > We have had this discussion before but you still sent the same exact > bad changelog. Actually you said:- > 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. And:- > 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. So it wasn't clear *to me* you wanted me to change that, given you asked me *not* to redo it explicitly (which I assumed applied to the message too) - apologies if I misinterpreted this! Best,
On 18 March 2015 at 10:17, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Why is there a RESEND in the subject.
To avoid confusion (and Sudip explicitly mentioned there might be
some), and in addition I had to update my patch series to take into
account that it no longer applied due to another patch which applies
the ANSI C function prototype fixes having already been applied.
Best,
Lorenzo Stoakes <lstoakes@gmail.com> writes: > 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(-) > > 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); ERROR is reported by scripts/checkpatch.pl (spaces are missing after ','). This coding style problem was there before your patch but I don't think it makes sense to preserve it. > 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); the same > 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); WARNING: please, no spaces at the start of a line #137: FILE: drivers/staging/sm750fb/sm750.c:833: + 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); the same missing spaces > > pr_info("sm%3x mmio address = %p\n",share->devid,share->pvReg); > > -- > 2.3.2 > _______________________________________________ > devel mailing list > devel@linuxdriverproject.org > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
On Wed, Mar 18, 2015 at 10:44:53AM +0000, Lorenzo Stoakes wrote: > On 18 March 2015 at 10:18, Dan Carpenter <dan.carpenter@oracle.com> wrote: > >> > >> This changelog still sucks. It doesn't describe the effect of this > >> behavior change for the user. It doesn't even make it clear that you > >> are aware that this is a behavior change. > > > > It doesn't say to me that you have asked yourself if the sparse > > annotations are correct. Many times they are wrong. > > My understanding, which as a new contributor is of course limited and > likely simply wrong in many aspects, is - these memset's are referring > to I/O mapped memory, which as far as I can tell is actually the case > here, so in order to make it explicit that this is the case and we > know it is, we use memset_io. As far as I understand the pointers > simply have a modifier applied which marks them as I/O mapped memory > for the purposes of sparse checking whether they are used consistently > as such and are not treated like they are a normal kernel pointer. > > In this case the cursor->vstart and crtc->vScreen pointers, looking > through the source, explicitly refer to memory which is I/O mapped, > and is annotated as __iomem accordingly throughout. > > I will update the message accordingly, obviously if I'm > misunderstanding something let me know. This is all fine. > > > We have had this discussion before but you still sent the same exact > > bad changelog. > > Actually you said:- > > > 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. > > And:- The patch is fine. The changelog is not. > > > 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. > > So it wasn't clear *to me* you wanted me to change that, given you > asked me *not* to redo it explicitly (which I assumed applied to the > message too) - apologies if I misinterpreted this! I often say "don't resend" because something is minor and I don't want to slow you down but since you were resending it anyway then please fix it. Also changelogs are really easy to fix. In mutt, you can do it without leaving your email client. So I have revised my earlier statement, please fix it. :) 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 18 March 2015 at 10:52, Dan Carpenter <dan.carpenter@oracle.com> wrote: > I often say "don't resend" because something is minor and I don't want > to slow you down but since you were resending it anyway then please fix > it. Also changelogs are really easy to fix. In mutt, you can do it > without leaving your email client. So I have revised my earlier > statement, please fix it. :) Will do! I have experimented with mutt, I tend to just use git send-email directly at the moment and edit patch files manually and use the gmail web interface for reading/discussion, I am sure I will learn to do things a little more sanely in time, the hard way :) Best,
On Wed, Mar 18, 2015 at 10:46:52AM +0000, Lorenzo Stoakes wrote: > On 18 March 2015 at 10:17, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > Why is there a RESEND in the subject. > > To avoid confusion (and Sudip explicitly mentioned there might be > some), and in addition I had to update my patch series to take into > account that it no longer applied due to another patch which applies > the ANSI C function prototype fixes having already been applied. Btw, sorry for coming down hard on you. You're a newbie and expected to make these mistakes. Your patches are good and appreciated. Call it a v2 patch. Put a note under the --- cut off line: v2: updated to apply to latest linux-next http://kernelnewbies.org/PatchTipsAndTricks 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 18 March 2015 at 10:50, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > ERROR is reported by scripts/checkpatch.pl (spaces are missing after > ','). This coding style problem was there before your patch but I don't > think it makes sense to preserve it. [snip] > WARNING: please, no spaces at the start of a line > #137: FILE: drivers/staging/sm750fb/sm750.c:833: [snip] Hi Vitaly, these style issues have vexed me and I was not sure whether to make changes or preserve all the obvious errors so as not to blend the two changes inappropriately, however it does indeed make sense to fix these on the lines I'm changing, will fix these! Best,
On 18 March 2015 at 10:59, Dan Carpenter <dan.carpenter@oracle.com> wrote: > Btw, sorry for coming down hard on you. You're a newbie and expected to > make these mistakes. Your patches are good and appreciated. Thanks and no problem, I expect to receive robust criticism given the high standards in the kernel and see it as a means to improve my contributions :) > > Call it a v2 patch. Put a note under the --- cut off line: > v2: updated to apply to latest linux-next > > http://kernelnewbies.org/PatchTipsAndTricks > Great, thanks, will do! Very useful link! Best,
On Wed, Mar 18, 2015 at 11:12:20AM +0000, Lorenzo Stoakes wrote: > On 18 March 2015 at 10:50, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > ERROR is reported by scripts/checkpatch.pl (spaces are missing after > > ','). This coding style problem was there before your patch but I don't > > think it makes sense to preserve it. > > [snip] > > > WARNING: please, no spaces at the start of a line > > #137: FILE: drivers/staging/sm750fb/sm750.c:833: > > [snip] > > Hi Vitaly, these style issues have vexed me and I was not sure whether > to make changes or preserve all the obvious errors so as not to blend > the two changes inappropriately, however it does indeed make sense to > fix these on the lines I'm changing, will fix these! If it's a white space thing on the same line then it's generally ok to fix it. The "one thing per patch" is meant to make patches easier to review. If it's a trivial thing and it doesn't make it harder to review then we are reasonable people. Could you read your patches again and find other similar white space issues. + void __iomem * pbuffer,*pstart; Should be: + void __iomem *pbuffer, *pstart; 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 18, 2015 at 02:25:09PM +0300, Dan Carpenter wrote: > On Wed, Mar 18, 2015 at 11:12:20AM +0000, Lorenzo Stoakes wrote: > > On 18 March 2015 at 10:50, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > If it's a white space thing on the same line then it's generally ok to > fix it. The "one thing per patch" is meant to make patches easier to > review. If it's a trivial thing and it doesn't make it harder to review > then we are reasonable people. > but Greg K-H has explisitely mentiond not to do so. I did just that and fixed few whitespace things in the patch to fix the build failure. https://lkml.org/lkml/2015/3/10/685 regards sudip -- 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 18, 2015 at 06:36:07PM +0530, Sudip Mukherjee wrote: > On Wed, Mar 18, 2015 at 02:25:09PM +0300, Dan Carpenter wrote: > > On Wed, Mar 18, 2015 at 11:12:20AM +0000, Lorenzo Stoakes wrote: > > > On 18 March 2015 at 10:50, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > If it's a white space thing on the same line then it's generally ok to > > fix it. The "one thing per patch" is meant to make patches easier to > > review. If it's a trivial thing and it doesn't make it harder to review > > then we are reasonable people. > > > but Greg K-H has explisitely mentiond not to do so. > I did just that and fixed few whitespace things in the patch to fix the > build failure. > > https://lkml.org/lkml/2015/3/10/685 > You were making random white space changes and not on the same line. It was hard to review because you had to count how many u32 arguments there were (a million) and really look at it to see what the compile warning was. There was no compile warning in the end. Very annoying. 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 18, 2015 at 04:23:39PM +0300, Dan Carpenter wrote: > > https://lkml.org/lkml/2015/3/10/685 > > > > You were making random white space changes and not on the same line. It > was hard to review because you had to count how many u32 arguments there > were (a million) and really look at it to see what the compile warning > was. There was no compile warning in the end. Very annoying. ok, now understood. Thanks. like you said once - combining different changes in a single patch is an art. :) regards sudip > > 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 18 March 2015 at 10:59, Dan Carpenter <dan.carpenter@oracle.com> wrote: > Call it a v2 patch. Put a note under the --- cut off line: > v2: updated to apply to latest linux-next > > http://kernelnewbies.org/PatchTipsAndTricks Since each changed patch in the resend already incorporates changes to update to apply to the latest linux-next, I instead referenced whitespace/description changes (let me know if this isn't usually something you'd reference) to avoid confusion, as otherwise I'd have to resend other patches unchanged with just a v2 applied, then additionally update the ones that need changes. Let me know if you'd like this done differently! Best,
On 18 March 2015 at 11:25, Dan Carpenter <dan.carpenter@oracle.com> wrote: > Could you read your patches again and find other similar white space > issues. Done. 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(-) -- 2.3.2 -- 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