From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Bode Subject: Re: [PATCH] drm/tilcdc: Allocate register storage based on the actual number registers Date: Fri, 03 Jul 2015 23:10:47 +0200 Message-ID: <5596FA57.8070400@bumbleB.de> References: <1435920328-22822-1-git-send-email-jsarha@ti.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1229460729==" Return-path: Received: from server57.greatnet.de (server57.greatnet.de [178.254.50.57]) by gabe.freedesktop.org (Postfix) with ESMTPS id 3BCCA6E237 for ; Fri, 3 Jul 2015 14:10:55 -0700 (PDT) In-Reply-To: <1435920328-22822-1-git-send-email-jsarha@ti.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Jyri Sarha , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org This is a multi-part message in MIME format. --===============1229460729== Content-Type: multipart/alternative; boundary="------------030308050709050103000505" This is a multi-part message in MIME format. --------------030308050709050103000505 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Reviewed-By: Michael Bode Jyri, good idea to allocate the space for registers dynamically. This eliminates the need of synchronizing driver and header file in an elegant way. Only thing I personally don't like, is potentially passing a NULL pointer to kfree. But I learned that this is valid kernel code. Br, Michael On 07/03/2015 12:45 PM, Jyri Sarha wrote: > Allocate suspend/resume register storage based on the actual number > registers the driver is aware of. The static allocation for register > storage had falen behind badly. > > Reported-by: Michael Bode > Signed-off-by: Jyri Sarha > --- > drivers/gpu/drm/tilcdc/tilcdc_drv.c | 20 +++++++++++++++++++- > drivers/gpu/drm/tilcdc/tilcdc_drv.h | 2 +- > 2 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c > index 4908c1f..2f87263 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c > @@ -139,11 +139,14 @@ static int tilcdc_unload(struct drm_device *dev) > > pm_runtime_disable(dev->dev); > > + kfree(priv->saved_register); > kfree(priv); > > return 0; > } > > +static size_t tilcdc_num_regs(void); > + > static int tilcdc_load(struct drm_device *dev, unsigned long flags) > { > struct platform_device *pdev = dev->platformdev; > @@ -155,7 +158,11 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags) > int ret; > > priv = kzalloc(sizeof(*priv), GFP_KERNEL); > - if (!priv) { > + if (priv) > + priv->saved_register = kcalloc(sizeof(*priv->saved_register), > + tilcdc_num_regs(), GFP_KERNEL); > + if (!priv || !priv->saved_register) { > + kfree(priv); > dev_err(dev->dev, "failed to allocate private data\n"); > return -ENOMEM; > } > @@ -345,6 +352,7 @@ fail_free_wq: > > fail_free_priv: > dev->dev_private = NULL; > + kfree(priv->saved_register); > kfree(priv); > return ret; > } > @@ -467,6 +475,16 @@ static const struct { > REG(2, true, LCDC_INT_ENABLE_SET_REG), > #undef REG > }; > + > +static size_t tilcdc_num_regs(void) > +{ > + return ARRAY_SIZE(registers); > +} > +#else > +static size_t tilcdc_num_regs(void) > +{ > + return 0; > +} > #endif > > #ifdef CONFIG_DEBUG_FS > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h > index e863ad0..bc94835 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h > +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h > @@ -67,7 +67,7 @@ struct tilcdc_drm_private { > uint32_t max_width; > > /* register contents saved across suspend/resume: */ > - u32 saved_register[12]; > + u32 *saved_register; > > #ifdef CONFIG_CPU_FREQ > struct notifier_block freq_transition; --------------030308050709050103000505 Content-Type: text/html; charset=windows-1252 Content-Transfer-Encoding: 7bit


Reviewed-By: Michael Bode <michael@bumbleB.de>

Jyri,

good idea to allocate the space for registers dynamically.
This eliminates the need of synchronizing driver and header file in an elegant way.

Only thing I personally don't like, is potentially passing a NULL pointer to kfree.
But I learned that this is valid kernel code.

Br,
Michael



On 07/03/2015 12:45 PM, Jyri Sarha wrote:
Allocate suspend/resume register storage based on the actual number
registers the driver is aware of. The static allocation for register
storage had falen behind badly.

Reported-by: Michael Bode <michael@bumbleB.de>
Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 20 +++++++++++++++++++-
 drivers/gpu/drm/tilcdc/tilcdc_drv.h |  2 +-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 4908c1f..2f87263 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -139,11 +139,14 @@ static int tilcdc_unload(struct drm_device *dev)
 
 	pm_runtime_disable(dev->dev);
 
+	kfree(priv->saved_register);
 	kfree(priv);
 
 	return 0;
 }
 
+static size_t tilcdc_num_regs(void);
+
 static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 {
 	struct platform_device *pdev = dev->platformdev;
@@ -155,7 +158,11 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 	int ret;
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
-	if (!priv) {
+	if (priv)
+		priv->saved_register = kcalloc(sizeof(*priv->saved_register),
+					       tilcdc_num_regs(), GFP_KERNEL);
+	if (!priv || !priv->saved_register) {
+		kfree(priv);
 		dev_err(dev->dev, "failed to allocate private data\n");
 		return -ENOMEM;
 	}
@@ -345,6 +352,7 @@ fail_free_wq:
 
 fail_free_priv:
 	dev->dev_private = NULL;
+	kfree(priv->saved_register);
 	kfree(priv);
 	return ret;
 }
@@ -467,6 +475,16 @@ static const struct {
 		REG(2, true,  LCDC_INT_ENABLE_SET_REG),
 #undef REG
 };
+
+static size_t tilcdc_num_regs(void)
+{
+	return ARRAY_SIZE(registers);
+}
+#else
+static size_t tilcdc_num_regs(void)
+{
+	return 0;
+}
 #endif
 
 #ifdef CONFIG_DEBUG_FS
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
index e863ad0..bc94835 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
@@ -67,7 +67,7 @@ struct tilcdc_drm_private {
 	uint32_t max_width;
 
 	/* register contents saved across suspend/resume: */
-	u32 saved_register[12];
+	u32 *saved_register;
 
 #ifdef CONFIG_CPU_FREQ
 	struct notifier_block freq_transition;

--------------030308050709050103000505-- --===============1229460729== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============1229460729==--