[PATCH RESEND 1/5] staging: sm750fb: Use memset_io instead of memset

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 <redacted>
---
 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);
 		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);

--

2.3.2

This patch declares externally unavailable functions static. This fixes
the following sparse warnings:-

drivers/staging/sm750fb/ddk750_swi2c.c:223:6: warning: symbol 'swI2CStart' was not declared. Should it be static?
drivers/staging/sm750fb/ddk750_swi2c.c:234:6: warning: symbol 'swI2CStop' was not declared. Should it be static?
drivers/staging/sm750fb/ddk750_swi2c.c:252:6: warning: symbol 'swI2CWriteByte' was not declared. Should it be static?
drivers/staging/sm750fb/ddk750_swi2c.c:320:15: warning: symbol 'swI2CReadByte' was not declared. Should it be static?
drivers/staging/sm750fb/ddk750_swi2c.c:361:6: warning: symbol 'swI2CInit_SM750LE' was not declared. Should it be static?
drivers/staging/sm750fb/ddk750_hwi2c.c:63:6: warning: symbol 'hwI2CWaitTXDone' was not declared. Should it be static?
drivers/staging/sm750fb/ddk750_hwi2c.c:93:14: warning: symbol 'hwI2CWriteData' was not declared. Should it be static?
drivers/staging/sm750fb/ddk750_hwi2c.c:160:14: warning: symbol 'hwI2CReadData' was not declared. Should it be static?

Signed-off-by: Lorenzo Stoakes <redacted>
---
 drivers/staging/sm750fb/ddk750_hwi2c.c |  6 +++---
 drivers/staging/sm750fb/ddk750_swi2c.c | 10 +++++-----
 drivers/staging/sm750fb/sm750_accel.c  |  2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/sm750fb/ddk750_hwi2c.c b/drivers/staging/sm750fb/ddk750_hwi2c.c
index 84dfb6f..7826376 100644
--- a/drivers/staging/sm750fb/ddk750_hwi2c.c
+++ b/drivers/staging/sm750fb/ddk750_hwi2c.c

@@ -60,7 +60,7 @@ void hwI2CClose(void)
 }


-long hwI2CWaitTXDone(void)
+static long hwI2CWaitTXDone(void)
 {
     unsigned int timeout;

@@ -90,7 +90,7 @@ long hwI2CWaitTXDone(void)
  *  Return Value:
  *      Total number of bytes those are actually written.
  */
-unsigned int hwI2CWriteData(
+static unsigned int hwI2CWriteData(
     unsigned char deviceAddress,
     unsigned int length,
     unsigned char *pBuffer

@@ -157,7 +157,7 @@ unsigned int hwI2CWriteData(
  *  Return Value:
  *      Total number of actual bytes read from the slave device
  */
-unsigned int hwI2CReadData(
+static unsigned int hwI2CReadData(
     unsigned char deviceAddress,
     unsigned int length,
     unsigned char *pBuffer

diff --git a/drivers/staging/sm750fb/ddk750_swi2c.c b/drivers/staging/sm750fb/ddk750_swi2c.c
index 1249759..516f5bb 100644
--- a/drivers/staging/sm750fb/ddk750_swi2c.c
+++ b/drivers/staging/sm750fb/ddk750_swi2c.c

@@ -220,7 +220,7 @@ static void swI2CAck(void)
 /*
  *  This function sends the start command to the slave device
  */
-void swI2CStart(void)
+static void swI2CStart(void)
 {
     /* Start I2C */
     swI2CSDA(1);

@@ -231,7 +231,7 @@ void swI2CStart(void)
 /*
  *  This function sends the stop command to the slave device
  */
-void swI2CStop(void)
+static void swI2CStop(void)
 {
     /* Stop the I2C */
     swI2CSCL(1);

@@ -249,7 +249,7 @@ void swI2CStop(void)
  *       0   - Success
  *      -1   - Fail to write byte
  */
-long swI2CWriteByte(unsigned char data)
+static long swI2CWriteByte(unsigned char data)
 {
     unsigned char value = data;
     int i;

@@ -317,7 +317,7 @@ long swI2CWriteByte(unsigned char data)
  *  Return Value:
  *      One byte data read from the Slave device
  */
-unsigned char swI2CReadByte(unsigned char ack)
+static unsigned char swI2CReadByte(unsigned char ack)
 {
     int i;
     unsigned char data = 0;

@@ -358,7 +358,7 @@ unsigned char swI2CReadByte(unsigned char ack)
  *      -1   - Fail to initialize the i2c
  *       0   - Success
  */
-long swI2CInit_SM750LE(
+static long swI2CInit_SM750LE(
     unsigned char i2cClkGPIO,
     unsigned char i2cDataGPIO
 )
--

2.3.2

This patch removes the unused hw712_fillrect function. This patch fixes
the following sparse warning:-

drivers/staging/sm750fb/sm750_accel.c:95:5: warning: symbol 'hw712_fillrect' was not declared. Should it be static?

Signed-off-by: Lorenzo Stoakes <redacted>
---
 drivers/staging/sm750fb/sm750_accel.c | 78 -----------------------------------
 1 file changed, 78 deletions(-)

diff --git a/drivers/staging/sm750fb/sm750_accel.c b/drivers/staging/sm750fb/sm750_accel.c
index 6521c3b..c5a3726 100644
--- a/drivers/staging/sm750fb/sm750_accel.c
+++ b/drivers/staging/sm750fb/sm750_accel.c

@@ -89,84 +89,6 @@ void hw_set2dformat(struct lynx_accel * accel,int fmt)
 	write_dpr(accel,DE_STRETCH_FORMAT,reg);
 }

-/* seems sm712 RectFill command is broken,so need use BitBlt to
- * replace it. */
-
-int hw712_fillrect(struct lynx_accel * accel,
-				u32 base,u32 pitch,u32 Bpp,
-				u32 x,u32 y,u32 width,u32 height,
-				u32 color,u32 rop)
-{
-	u32 deCtrl;
-	if(accel->de_wait() != 0)
-	{
-		/* int time wait and always busy,seems hardware
-		 * got something error */
-		pr_debug("%s:De engine always bussy\n",__func__);
-		return -1;
-	}
-	/* 24bpp 2d acceleration still not work,we already support 2d on
-	 * both 8/16/32 bpp now, so there is no harm if we disable 2d on
-	 * 24bpp for current stage. */
-#if 0
-	if(Bpp = 3){
-		width *= 3;
-		x *= 3;
-		write_dpr(accel,DE_PITCH,
-			FIELD_VALUE(0,DE_PITCH,DESTINATION,pitch)|
-			FIELD_VALUE(0,DE_PITCH,SOURCE,pitch));//dpr10
-	}
-	else
-#endif
-	{
-		write_dpr(accel,DE_PITCH,
-			FIELD_VALUE(0,DE_PITCH,DESTINATION,pitch/Bpp)|
-			FIELD_VALUE(0,DE_PITCH,SOURCE,pitch/Bpp));//dpr10
-
-	}
-
-	write_dpr(accel,DE_FOREGROUND,color);//DPR14
-	write_dpr(accel,DE_MONO_PATTERN_HIGH,~0);//DPR34
-	write_dpr(accel,DE_MONO_PATTERN_LOW,~0);//DPR38
-
-	write_dpr(accel,DE_WINDOW_SOURCE_BASE,base);//dpr44
-	write_dpr(accel,DE_WINDOW_DESTINATION_BASE,base);//dpr40
-
-
-	write_dpr(accel,DE_WINDOW_WIDTH,
-			FIELD_VALUE(0,DE_WINDOW_WIDTH,DESTINATION,pitch/Bpp)|
-			FIELD_VALUE(0,DE_WINDOW_WIDTH,SOURCE,pitch/Bpp));//dpr3c
-
-
-	write_dpr(accel,DE_DESTINATION,
-			FIELD_SET(0,DE_DESTINATION,WRAP,DISABLE)|
-			FIELD_VALUE(0,DE_DESTINATION,X,x)|
-			FIELD_VALUE(0,DE_DESTINATION,Y,y));//dpr4
-
-	write_dpr(accel,DE_DIMENSION,
-			FIELD_VALUE(0,DE_DIMENSION,X,width)|
-			FIELD_VALUE(0,DE_DIMENSION,Y_ET,height));//dpr8
-
-	deCtrl -		FIELD_SET(0,DE_CONTROL,STATUS,START)|
-		FIELD_SET(0,DE_CONTROL,COMMAND,BITBLT)|
-		FIELD_SET(0,DE_CONTROL,ROP2_SOURCE,PATTERN)|
-		FIELD_SET(0,DE_CONTROL,ROP_SELECT,ROP2)|
-		FIELD_VALUE(0,DE_CONTROL,ROP,rop);//dpr0xc
-#if 0
-		/* dump registers */
-		int i;
-		inf_msg("x,y,w,h = %d,%d,%d,%d\n",x,y,width,height);
-		for(i=0x04;i<=0x44;i+=4){
-			inf_msg("dpr%02x = %08x\n",i,read_dpr(accel,i));
-		}
-		inf_msg("deCtrl = %08x\n",deCtrl);
-#endif
-
-	write_dpr(accel,DE_CONTROL,deCtrl);
-	return 0;
-}
-
 int hw_fillrect(struct lynx_accel * accel,
 				u32 base,u32 pitch,u32 Bpp,
 				u32 x,u32 y,u32 width,u32 height,
--

2.3.2

This patch annotates pointers as referring to I/O mapped memory where they ought
to be, removes now unnecessary ugly casts, eliminates an incorrect deref on I/O
mapped memory by using iowrite16 instead, and updates the pointer arithmetic
accordingly to take into account that the pointers are now byte-sized. This
fixes the following sparse warnings:-

drivers/staging/sm750fb/sm750_cursor.c:113:19: warning: cast removes address space of expression
drivers/staging/sm750fb/sm750_cursor.c:204:19: warning: cast removes address space of expression

Signed-off-by: Lorenzo Stoakes <redacted>
---
 drivers/staging/sm750fb/sm750_cursor.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/sm750fb/sm750_cursor.c b/drivers/staging/sm750fb/sm750_cursor.c
index 6cceef1..c2ff3bd 100644
--- a/drivers/staging/sm750fb/sm750_cursor.c
+++ b/drivers/staging/sm750fb/sm750_cursor.c

@@ -98,7 +98,7 @@ void hw_cursor_setData(struct lynx_cursor * cursor,
 	int i,j,count,pitch,offset;
 	u8 color,mask,opr;
 	u16 data;
-	u16 * pbuffer,*pstart;
+	void __iomem * pbuffer,*pstart;

 	/*  in byte*/
 	pitch = cursor->w >> 3;

@@ -106,11 +106,11 @@ void hw_cursor_setData(struct lynx_cursor * cursor,
 	/* in byte	*/
 	count = pitch * cursor->h;

-	/* in ushort */
-	offset = cursor->maxW * 2 / 8 / 2;
+	/* in byte */
+	offset = cursor->maxW * 2 / 8;

 	data = 0;
-	pstart = (u16 *)cursor->vstart;
+	pstart = cursor->vstart;
 	pbuffer = pstart;

 /*

@@ -161,7 +161,7 @@ void hw_cursor_setData(struct lynx_cursor * cursor,
 			}
 		}
 #endif
-		*pbuffer = data;
+		iowrite16(data, pbuffer);

 		/* assume pitch is 1,2,4,8,...*/
 #if 0

@@ -174,7 +174,7 @@ void hw_cursor_setData(struct lynx_cursor * cursor,
 			pstart += offset;
 			pbuffer = pstart;
 		}else{
-			pbuffer++;
+			pbuffer += sizeof(u16);
 		}

 	}

@@ -189,7 +189,7 @@ void hw_cursor_setData2(struct lynx_cursor * cursor,
 	int i,j,count,pitch,offset;
 	u8 color, mask;
 	u16 data;
-	u16 * pbuffer,*pstart;
+	void __iomem * pbuffer,*pstart;

 	/*  in byte*/
 	pitch = cursor->w >> 3;

@@ -197,11 +197,11 @@ void hw_cursor_setData2(struct lynx_cursor * cursor,
 	/* in byte	*/
 	count = pitch * cursor->h;

-	/* in ushort */
-	offset = cursor->maxW * 2 / 8 / 2;
+	/* in byte */
+	offset = cursor->maxW * 2 / 8;

 	data = 0;
-	pstart = (u16 *)cursor->vstart;
+	pstart = cursor->vstart;
 	pbuffer = pstart;

 	for(i=0;i<count;i++)

@@ -234,7 +234,7 @@ void hw_cursor_setData2(struct lynx_cursor * cursor,
 				data |= ((color & (1<<j))?1:2)<<(j*2);
 		}
 #endif
-		*pbuffer = data;
+		iowrite16(data, pbuffer);

 		/* assume pitch is 1,2,4,8,...*/
 		if(!(i&(pitch-1)))

@@ -244,7 +244,7 @@ void hw_cursor_setData2(struct lynx_cursor * cursor,
 			pstart += offset;
 			pbuffer = pstart;
 		}else{
-			pbuffer++;
+			pbuffer += sizeof(u16);
 		}

 	}
--

2.3.2

This patch removes the unnecessary spinlock helper function and instead
calls spin_lock and spin_unlock directly.

This does *not* resolve sparse warnings about context imbalances but these are
spurious.

Signed-off-by: Lorenzo Stoakes <redacted>
---
 drivers/staging/sm750fb/sm750.c | 53 +++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index be35429..a6658e1 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c

@@ -56,23 +56,6 @@ static char * g_settings = NULL;
 static int g_dualview;
 static char * g_option = NULL;

-/* if not use spin_lock,system will die if user load driver
- * and immediatly unload driver frequently (dual)*/
-static inline void myspin_lock(spinlock_t * sl){
-	struct lynx_share * share;
-	share = container_of(sl,struct lynx_share,slock);
-	if(share->dual){
-		spin_lock(sl);
-	}
-}
-
-static inline void myspin_unlock(spinlock_t * sl){
-	struct lynx_share * share;
-	share = container_of(sl,struct lynx_share,slock);
-	if(share->dual){
-		spin_unlock(sl);
-	}
-}
 static const struct fb_videomode lynx750_ext[] = {
 	/*  	1024x600-60 VESA 	[1.71:1]	*/
 	{NULL,  60, 1024, 600, 20423, 144,  40, 18, 1, 104, 3,

@@ -209,13 +192,21 @@ static void lynxfb_ops_fillrect(struct fb_info* info,const struct fb_fillrect* r
 	color = (Bpp = 1)?region->color:((u32*)info->pseudo_palette)[region->color];
 	rop = ( region->rop != ROP_COPY ) ? HW_ROP2_XOR:HW_ROP2_COPY;

-	myspin_lock(&share->slock);
+	/*
+	 * If not use spin_lock,system will die if user load driver
+	 * and immediatly unload driver frequently (dual)
+	 */
+	if (share->dual)
+		spin_lock(&share->slock);
+
 	share->accel.de_fillrect(&share->accel,
 							base,pitch,Bpp,
 							region->dx,region->dy,
 							region->width,region->height,
 							color,rop);
-	myspin_unlock(&share->slock);
+
+	if (share->dual)
+		spin_unlock(&share->slock);
 }

 static void lynxfb_ops_copyarea(struct fb_info * info,const struct fb_copyarea * region)

@@ -233,12 +224,20 @@ static void lynxfb_ops_copyarea(struct fb_info * info,const struct fb_copyarea *
 	pitch = info->fix.line_length;
 	Bpp = info->var.bits_per_pixel >> 3;

-	myspin_lock(&share->slock);
+	/*
+	 * If not use spin_lock, system will die if user load driver
+	 * and immediatly unload driver frequently (dual)
+	 */
+	if (share->dual)
+		spin_lock(&share->slock);
+
 	share->accel.de_copyarea(&share->accel,
 							base,pitch,region->sx,region->sy,
 							base,pitch,Bpp,region->dx,region->dy,
 							region->width,region->height,HW_ROP2_COPY);
-	myspin_unlock(&share->slock);
+
+	if (share->dual)
+		spin_unlock(&share->slock);
 }

 static void lynxfb_ops_imageblit(struct fb_info*info,const struct fb_image* image)

@@ -272,14 +271,22 @@ static void lynxfb_ops_imageblit(struct fb_info*info,const struct fb_image* imag
 	}
 	return;
 _do_work:
-	myspin_lock(&share->slock);
+	/*
+	 * If not use spin_lock, system will die if user load driver
+	 * and immediatly unload driver frequently (dual)
+	 */
+	if (share->dual)
+		spin_lock(&share->slock);
+
 	share->accel.de_imageblit(&share->accel,
 					image->data,image->width>>3,0,
 					base,pitch,Bpp,
 					image->dx,image->dy,
 					image->width,image->height,
 					fgcol,bgcol,HW_ROP2_COPY);
-	myspin_unlock(&share->slock);
+
+	if (share->dual)
+		spin_unlock(&share->slock);
 }

 static int lynxfb_ops_pan_display(struct fb_var_screeninfo *var,
--

2.3.3

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

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:

quoted

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

On 18 March 2015 at 10:18, Dan Carpenter [off-list ref] wrote:

quoted

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,

-- 
Lorenzo Stoakes
https:/ljs.io

On 18 March 2015 at 10:17, Dan Carpenter [off-list ref] 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
https:/ljs.io

On Wed, Mar 18, 2015 at 10:44:53AM +0000, Lorenzo Stoakes wrote:

On 18 March 2015 at 10:18, Dan Carpenter [off-list ref] wrote:

quoted

quoted

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.

quoted

We have had this discussion before but you still sent the same exact
bad changelog.

Actually you said:-

quoted

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.

quoted

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

On 18 March 2015 at 10:52, Dan Carpenter [off-list ref] 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,

-- 
Lorenzo Stoakes
https:/ljs.io

On Wed, Mar 18, 2015 at 10:46:52AM +0000, Lorenzo Stoakes wrote:

On 18 March 2015 at 10:17, Dan Carpenter [off-list ref] wrote:

quoted

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

On 18 March 2015 at 10:59, Dan Carpenter [off-list ref] 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,

-- 
Lorenzo Stoakes
https:/ljs.io

On 18 March 2015 at 10:59, Dan Carpenter [off-list ref] 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,

-- 
Lorenzo Stoakes
https:/ljs.io