diff mbox series

[2/5] Fonts: Make font size unsigned in font_desc

Message ID 54f7d42e07eca2a2f13669575a9de88023ebc1ac.1603788512.git.yepeilin.cs@gmail.com (mailing list archive)
State Not Applicable
Headers show
Series None | expand

Commit Message

Peilin Ye Oct. 27, 2020, 4:33 p.m. UTC
It is improper to define `width` and `height` as signed in `struct
font_desc`. Make them unsigned. Also, change the corresponding printk()
format identifiers from `%d` to `%u`, in sti_select_fbfont().

Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
Build-tested.

 drivers/video/console/sticore.c | 2 +-
 include/linux/font.h            | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Daniel Vetter Oct. 27, 2020, 6:50 p.m. UTC | #1
On Tue, Oct 27, 2020 at 12:33:05PM -0400, Peilin Ye wrote:
> It is improper to define `width` and `height` as signed in `struct
> font_desc`. Make them unsigned. Also, change the corresponding printk()
> format identifiers from `%d` to `%u`, in sti_select_fbfont().
> 
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>

I'm not entirely sure of the motivation here ... height/width should never
ever be even close to the limit here. Or have you seen integer math that
could potentially go wrong if we go with unsigned instead of int?
-Daniel

> ---
> Build-tested.
> 
>  drivers/video/console/sticore.c | 2 +-
>  include/linux/font.h            | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/console/sticore.c b/drivers/video/console/sticore.c
> index 6a26a364f9bd..d1bb5915082b 100644
> --- a/drivers/video/console/sticore.c
> +++ b/drivers/video/console/sticore.c
> @@ -502,7 +502,7 @@ sti_select_fbfont(struct sti_cooked_rom *cooked_rom, const char *fbfont_name)
>  	if (!fbfont)
>  		return NULL;
>  
> -	pr_info("STI selected %dx%d framebuffer font %s for sticon\n",
> +	pr_info("STI selected %ux%u framebuffer font %s for sticon\n",
>  			fbfont->width, fbfont->height, fbfont->name);
>  			
>  	bpc = ((fbfont->width+7)/8) * fbfont->height; 
> diff --git a/include/linux/font.h b/include/linux/font.h
> index b5b312c19e46..4f50d736ea72 100644
> --- a/include/linux/font.h
> +++ b/include/linux/font.h
> @@ -16,7 +16,7 @@
>  struct font_desc {
>      int idx;
>      const char *name;
> -    int width, height;
> +    unsigned int width, height;
>      const void *data;
>      int pref;
>  };
> -- 
> 2.25.1
>
Peilin Ye Oct. 28, 2020, 5:43 a.m. UTC | #2
On Tue, Oct 27, 2020 at 07:50:58PM +0100, Daniel Vetter wrote:
> On Tue, Oct 27, 2020 at 12:33:05PM -0400, Peilin Ye wrote:
> > It is improper to define `width` and `height` as signed in `struct
> > font_desc`. Make them unsigned. Also, change the corresponding printk()
> > format identifiers from `%d` to `%u`, in sti_select_fbfont().
> > 
> > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> 
> I'm not entirely sure of the motivation here ... height/width should never
> ever be even close to the limit here. Or have you seen integer math that
> could potentially go wrong if we go with unsigned instead of int?

Oh... No, I have not. I just thought we shouldn't represent a length
using a signed value. Also, width and height in console_font are
unsigned int - that shouldn't matter that much though.

[3/5] doesn't hunk properly without this patch, I'll send a v2 for [3/5]
soon.

Peilin
Daniel Vetter Oct. 28, 2020, 8:18 a.m. UTC | #3
On Wed, Oct 28, 2020 at 01:43:07AM -0400, Peilin Ye wrote:
> On Tue, Oct 27, 2020 at 07:50:58PM +0100, Daniel Vetter wrote:
> > On Tue, Oct 27, 2020 at 12:33:05PM -0400, Peilin Ye wrote:
> > > It is improper to define `width` and `height` as signed in `struct
> > > font_desc`. Make them unsigned. Also, change the corresponding printk()
> > > format identifiers from `%d` to `%u`, in sti_select_fbfont().
> > > 
> > > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> > 
> > I'm not entirely sure of the motivation here ... height/width should never
> > ever be even close to the limit here. Or have you seen integer math that
> > could potentially go wrong if we go with unsigned instead of int?
> 
> Oh... No, I have not. I just thought we shouldn't represent a length
> using a signed value. Also, width and height in console_font are
> unsigned int - that shouldn't matter that much though.

Oh this is actually a good reason, since that's the uapi structure. And so
using the exact same signedness should help a bit with accidental casting
bugs.

If you mention this in the commit message I think this is good to go.
-Daniel

> 
> [3/5] doesn't hunk properly without this patch, I'll send a v2 for [3/5]
> soon.
> 
> Peilin
>
Peilin Ye Oct. 28, 2020, 10:30 a.m. UTC | #4
On Wed, Oct 28, 2020 at 09:18:44AM +0100, Daniel Vetter wrote:
> On Wed, Oct 28, 2020 at 01:43:07AM -0400, Peilin Ye wrote:
> > On Tue, Oct 27, 2020 at 07:50:58PM +0100, Daniel Vetter wrote:
> > > On Tue, Oct 27, 2020 at 12:33:05PM -0400, Peilin Ye wrote:
> > > > It is improper to define `width` and `height` as signed in `struct
> > > > font_desc`. Make them unsigned. Also, change the corresponding printk()
> > > > format identifiers from `%d` to `%u`, in sti_select_fbfont().
> > > > 
> > > > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> > > 
> > > I'm not entirely sure of the motivation here ... height/width should never
> > > ever be even close to the limit here. Or have you seen integer math that
> > > could potentially go wrong if we go with unsigned instead of int?
> > 
> > Oh... No, I have not. I just thought we shouldn't represent a length
> > using a signed value. Also, width and height in console_font are
> > unsigned int - that shouldn't matter that much though.
> 
> Oh this is actually a good reason, since that's the uapi structure. And so
> using the exact same signedness should help a bit with accidental casting
> bugs.
> 
> If you mention this in the commit message I think this is good to go.

Ah, I see, v2 on the way. Please ignore [v2 3/5], that doesn't hunk with
this patch in effect...

One newbie question, should I mention in the commit message, if a patch
depends on another patch in the series in order to hunk properly?

Peilin
Daniel Vetter Oct. 28, 2020, 6:40 p.m. UTC | #5
On Wed, Oct 28, 2020 at 06:56:47AM -0400, Peilin Ye wrote:
> `width` and `height` are defined as unsigned in our UAPI font descriptor
> `struct console_font`. Make them unsigned in our kernel font descriptor
> `struct font_desc`, too.
> 
> Also, change the corresponding printk() format identifiers from `%d` to
> `%u`, in sti_select_fbfont().
> 
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>

Pushed to drm-misc-next, thanks for the patch.
-Daniel

> ---
> Change in v2:
>   - Mention `struct console_font` in the commit message. (Suggested by
>     Daniel Vetter <daniel@ffwll.ch>)
> 
>  drivers/video/console/sticore.c | 2 +-
>  include/linux/font.h            | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/console/sticore.c b/drivers/video/console/sticore.c
> index 6a26a364f9bd..d1bb5915082b 100644
> --- a/drivers/video/console/sticore.c
> +++ b/drivers/video/console/sticore.c
> @@ -502,7 +502,7 @@ sti_select_fbfont(struct sti_cooked_rom *cooked_rom, const char *fbfont_name)
>  	if (!fbfont)
>  		return NULL;
>  
> -	pr_info("STI selected %dx%d framebuffer font %s for sticon\n",
> +	pr_info("STI selected %ux%u framebuffer font %s for sticon\n",
>  			fbfont->width, fbfont->height, fbfont->name);
>  			
>  	bpc = ((fbfont->width+7)/8) * fbfont->height; 
> diff --git a/include/linux/font.h b/include/linux/font.h
> index b5b312c19e46..4f50d736ea72 100644
> --- a/include/linux/font.h
> +++ b/include/linux/font.h
> @@ -16,7 +16,7 @@
>  struct font_desc {
>      int idx;
>      const char *name;
> -    int width, height;
> +    unsigned int width, height;
>      const void *data;
>      int pref;
>  };
> -- 
> 2.25.1
>
diff mbox series

Patch

diff --git a/drivers/video/console/sticore.c b/drivers/video/console/sticore.c
index 6a26a364f9bd..d1bb5915082b 100644
--- a/drivers/video/console/sticore.c
+++ b/drivers/video/console/sticore.c
@@ -502,7 +502,7 @@  sti_select_fbfont(struct sti_cooked_rom *cooked_rom, const char *fbfont_name)
 	if (!fbfont)
 		return NULL;
 
-	pr_info("STI selected %dx%d framebuffer font %s for sticon\n",
+	pr_info("STI selected %ux%u framebuffer font %s for sticon\n",
 			fbfont->width, fbfont->height, fbfont->name);
 			
 	bpc = ((fbfont->width+7)/8) * fbfont->height; 
diff --git a/include/linux/font.h b/include/linux/font.h
index b5b312c19e46..4f50d736ea72 100644
--- a/include/linux/font.h
+++ b/include/linux/font.h
@@ -16,7 +16,7 @@ 
 struct font_desc {
     int idx;
     const char *name;
-    int width, height;
+    unsigned int width, height;
     const void *data;
     int pref;
 };