[PATCH 1/4] drivers/misc/sgi-gru: remove unused variable

[PATCH 1/4] drivers/misc/sgi-gru: remove unused variable

[Sudip Mukherjee]

These variables were only assigned some value and were never used.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

I have removed the variables in the functions gru_dump_tfm() and
gru_dump_tgh() but it appeared that the intended logic might have been
something like:
bytes = GRU_NUM_TFM * GRU_CACHE_LINE_BYTES;
if (bytes > ubufend - ubuf)
	return -EFBIG;

But since I was not sure so removing the variable was the safe choice as
it is not changing the logic.

 drivers/misc/sgi-gru/grukdump.c     | 12 ++----------
 drivers/misc/sgi-gru/grukservices.c |  2 --
 2 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/misc/sgi-gru/grukdump.c b/drivers/misc/sgi-gru/grukdump.c
index a3700a5..fa515e3 100644
--- a/drivers/misc/sgi-gru/grukdump.c
+++ b/drivers/misc/sgi-gru/grukdump.c
@@ -78,11 +78,7 @@ static int gru_dump_tfm(struct gru_state *gru,
 		void __user *ubuf, void __user *ubufend)
 {
 	struct gru_tlb_fault_map *tfm;
-	int i, ret, bytes;
-
-	bytes = GRU_NUM_TFM * GRU_CACHE_LINE_BYTES;
-	if (bytes > ubufend - ubuf)
-		ret = -EFBIG;
+	int i;
 
 	for (i = 0; i < GRU_NUM_TFM; i++) {
 		tfm = get_tfm(gru->gs_gru_base_vaddr, i);
@@ -99,11 +95,7 @@ static int gru_dump_tgh(struct gru_state *gru,
 		void __user *ubuf, void __user *ubufend)
 {
 	struct gru_tlb_global_handle *tgh;
-	int i, ret, bytes;
-
-	bytes = GRU_NUM_TGH * GRU_CACHE_LINE_BYTES;
-	if (bytes > ubufend - ubuf)
-		ret = -EFBIG;
+	int i;
 
 	for (i = 0; i < GRU_NUM_TGH; i++) {
 		tgh = get_tgh(gru->gs_gru_base_vaddr, i);
diff --git a/drivers/misc/sgi-gru/grukservices.c b/drivers/misc/sgi-gru/grukservices.c
index 913de07..1f0bdab 100644
--- a/drivers/misc/sgi-gru/grukservices.c
+++ b/drivers/misc/sgi-gru/grukservices.c
@@ -997,7 +997,6 @@ static int quicktest1(unsigned long arg)
 {
 	struct gru_message_queue_desc mqd;
 	void *p, *mq;
-	unsigned long *dw;
 	int i, ret = -EIO;
 	char mes[GRU_CACHE_LINE_BYTES], *m;
 
@@ -1007,7 +1006,6 @@ static int quicktest1(unsigned long arg)
 		return -ENOMEM;
 	mq = ALIGNUP(p, 1024);
 	memset(mes, 0xee, sizeof(mes));
-	dw = mq;
 
 	gru_create_message_queue(&mqd, mq, 8 * GRU_CACHE_LINE_BYTES, 0, 0, 0);
 	for (i = 0; i < 6; i++) {
-- 
1.9.1

[PATCH 2/4] drivers/misc/sgi-gru: make functions static

[Sudip Mukherjee]

The functions gru_get_cb_exception_detail_str() and gru_abort() were
only called locally from that file. We can make them static.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/misc/sgi-gru/grukservices.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/sgi-gru/grukservices.c b/drivers/misc/sgi-gru/grukservices.c
index 1f0bdab..a6fd773 100644
--- a/drivers/misc/sgi-gru/grukservices.c
+++ b/drivers/misc/sgi-gru/grukservices.c
@@ -429,8 +429,8 @@ int gru_get_cb_exception_detail(void *cb,
 	return 0;
 }
 
-char *gru_get_cb_exception_detail_str(int ret, void *cb,
-				      char *buf, int size)
+static char *gru_get_cb_exception_detail_str(int ret, void *cb,
+					     char *buf, int size)
 {
 	struct gru_control_block_status *gen = (void *)cb;
 	struct control_block_extended_exc_detail excdet;
@@ -505,7 +505,7 @@ int gru_wait_proc(void *cb)
 	return ret;
 }
 
-void gru_abort(int ret, void *cb, char *str)
+static void gru_abort(int ret, void *cb, char *str)
 {
 	char buf[GRU_EXC_STR_SIZE];
 
-- 
1.9.1

[PATCH 3/4] drivers/misc/sgi-gru: remove always false condition

[Sudip Mukherjee]

The member gid in struct gru_dump_chiplet_state_req is unsigned int. So
it can never be less than 0.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/misc/sgi-gru/grukdump.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/sgi-gru/grukdump.c b/drivers/misc/sgi-gru/grukdump.c
index fa515e3..dbba197 100644
--- a/drivers/misc/sgi-gru/grukdump.c
+++ b/drivers/misc/sgi-gru/grukdump.c
@@ -188,7 +188,7 @@ int gru_dump_chiplet_request(unsigned long arg)
 		return -EFAULT;
 
 	/* Currently, only dump by gid is implemented */
-	if (req.gid >= gru_max_gids || req.gid < 0)
+	if (req.gid >= gru_max_gids)
 		return -EINVAL;
 
 	gru = GID_TO_GRU(req.gid);
-- 
1.9.1

[PATCH 4/4] drivers/misc/sgi-gru: fix dereference of ERR_PTR

[Sudip Mukherjee]

gru_alloc_gts() can fail and it can return ERR_PTR(errvalue). We should
not dereference it if it has returned error. And incase it has returned
error then just downgrade the write lock and exit.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/misc/sgi-gru/grukservices.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/misc/sgi-gru/grukservices.c b/drivers/misc/sgi-gru/grukservices.c
index a6fd773..369f7fe 100644
--- a/drivers/misc/sgi-gru/grukservices.c
+++ b/drivers/misc/sgi-gru/grukservices.c
@@ -161,6 +161,8 @@ static void gru_load_kernel_context(struct gru_blade_state *bs, int blade_id)
 
 	if (!bs->bs_kgts) {
 		bs->bs_kgts = gru_alloc_gts(NULL, 0, 0, 0, 0, 0);
+		if (IS_ERR(bs->bs_kgts))
+			goto err_gru_alloc_gts;
 		bs->bs_kgts->ts_user_blade_id = blade_id;
 	}
 	kgts = bs->bs_kgts;
@@ -184,6 +186,7 @@ static void gru_load_kernel_context(struct gru_blade_state *bs, int blade_id)
 		bs->kernel_cb = get_gseg_base_address_cb(vaddr, ctxnum, 0);
 		bs->kernel_dsr = get_gseg_base_address_ds(vaddr, ctxnum, 0);
 	}
+err_gru_alloc_gts:
 	downgrade_write(&bs->bs_kgts_sema);
 }
 
-- 
1.9.1

Re: [PATCH 1/4] drivers/misc/sgi-gru: remove unused variable

[Dimitri Sivanich]

On Wed, Sep 02, 2015 at 04:54:55PM +0530, Sudip Mukherjee wrote:
> These variables were only assigned some value and were never used.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
> 
> I have removed the variables in the functions gru_dump_tfm() and
> gru_dump_tgh() but it appeared that the intended logic might have been
> something like:
> bytes = GRU_NUM_TFM * GRU_CACHE_LINE_BYTES;
> if (bytes > ubufend - ubuf)
> 	return -EFBIG;
> 
> But since I was not sure so removing the variable was the safe choice as
> it is not changing the logic.

I think better would be as shown below.
> 
>  drivers/misc/sgi-gru/grukdump.c     | 12 ++----------
>  drivers/misc/sgi-gru/grukservices.c |  2 --
>  2 files changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/misc/sgi-gru/grukdump.c b/drivers/misc/sgi-gru/grukdump.c
> index a3700a5..fa515e3 100644
> --- a/drivers/misc/sgi-gru/grukdump.c
> +++ b/drivers/misc/sgi-gru/grukdump.c
> @@ -78,11 +78,7 @@ static int gru_dump_tfm(struct gru_state *gru,
>  		void __user *ubuf, void __user *ubufend)
>  {
>  	struct gru_tlb_fault_map *tfm;
> -	int i, ret, bytes;
> -
> -	bytes = GRU_NUM_TFM * GRU_CACHE_LINE_BYTES;
> -	if (bytes > ubufend - ubuf)
> -		ret = -EFBIG;
> +	int i;
+
+ 	if ((GRU_NUM_TFM * GRU_CACHE_LINE_BYTES) > (ubufend - ubuf))
+		return -EFBIG;
>  
>  	for (i = 0; i < GRU_NUM_TFM; i++) {
>  		tfm = get_tfm(gru->gs_gru_base_vaddr, i);
> @@ -99,11 +95,7 @@ static int gru_dump_tgh(struct gru_state *gru,
>  		void __user *ubuf, void __user *ubufend)
>  {
>  	struct gru_tlb_global_handle *tgh;
> -	int i, ret, bytes;
> -
> -	bytes = GRU_NUM_TGH * GRU_CACHE_LINE_BYTES;
> -	if (bytes > ubufend - ubuf)
> -		ret = -EFBIG;
> +	int i;

(Same as shown above)
>  
>  	for (i = 0; i < GRU_NUM_TGH; i++) {
>  		tgh = get_tgh(gru->gs_gru_base_vaddr, i);
> diff --git a/drivers/misc/sgi-gru/grukservices.c b/drivers/misc/sgi-gru/grukservices.c
> index 913de07..1f0bdab 100644
> --- a/drivers/misc/sgi-gru/grukservices.c
> +++ b/drivers/misc/sgi-gru/grukservices.c
> @@ -997,7 +997,6 @@ static int quicktest1(unsigned long arg)
>  {
>  	struct gru_message_queue_desc mqd;
>  	void *p, *mq;
> -	unsigned long *dw;
>  	int i, ret = -EIO;
>  	char mes[GRU_CACHE_LINE_BYTES], *m;
>  
> @@ -1007,7 +1006,6 @@ static int quicktest1(unsigned long arg)
>  		return -ENOMEM;
>  	mq = ALIGNUP(p, 1024);
>  	memset(mes, 0xee, sizeof(mes));
> -	dw = mq;
>  
>  	gru_create_message_queue(&mqd, mq, 8 * GRU_CACHE_LINE_BYTES, 0, 0, 0);
>  	for (i = 0; i < 6; i++) {
> -- 
> 1.9.1

Re: [PATCH 2/4] drivers/misc/sgi-gru: make functions static

[Dimitri Sivanich]

Acked-by: Dimitri Sivanich <sivanich@sgi.com>

On Wed, Sep 02, 2015 at 04:54:56PM +0530, Sudip Mukherjee wrote:
> The functions gru_get_cb_exception_detail_str() and gru_abort() were
> only called locally from that file. We can make them static.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>  drivers/misc/sgi-gru/grukservices.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/misc/sgi-gru/grukservices.c b/drivers/misc/sgi-gru/grukservices.c
> index 1f0bdab..a6fd773 100644
> --- a/drivers/misc/sgi-gru/grukservices.c
> +++ b/drivers/misc/sgi-gru/grukservices.c
> @@ -429,8 +429,8 @@ int gru_get_cb_exception_detail(void *cb,
>  	return 0;
>  }
>  
> -char *gru_get_cb_exception_detail_str(int ret, void *cb,
> -				      char *buf, int size)
> +static char *gru_get_cb_exception_detail_str(int ret, void *cb,
> +					     char *buf, int size)
>  {
>  	struct gru_control_block_status *gen = (void *)cb;
>  	struct control_block_extended_exc_detail excdet;
> @@ -505,7 +505,7 @@ int gru_wait_proc(void *cb)
>  	return ret;
>  }
>  
> -void gru_abort(int ret, void *cb, char *str)
> +static void gru_abort(int ret, void *cb, char *str)
>  {
>  	char buf[GRU_EXC_STR_SIZE];
>  
> -- 
> 1.9.1

Re: [PATCH 3/4] drivers/misc/sgi-gru: remove always false condition

[Dimitri Sivanich]

Acked-by: Dimitri Sivanich <sivanich@sgi.com>

On Wed, Sep 02, 2015 at 04:54:57PM +0530, Sudip Mukherjee wrote:
> The member gid in struct gru_dump_chiplet_state_req is unsigned int. So
> it can never be less than 0.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>  drivers/misc/sgi-gru/grukdump.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/sgi-gru/grukdump.c b/drivers/misc/sgi-gru/grukdump.c
> index fa515e3..dbba197 100644
> --- a/drivers/misc/sgi-gru/grukdump.c
> +++ b/drivers/misc/sgi-gru/grukdump.c
> @@ -188,7 +188,7 @@ int gru_dump_chiplet_request(unsigned long arg)
>  		return -EFAULT;
>  
>  	/* Currently, only dump by gid is implemented */
> -	if (req.gid >= gru_max_gids || req.gid < 0)
> +	if (req.gid >= gru_max_gids)
>  		return -EINVAL;
>  
>  	gru = GID_TO_GRU(req.gid);
> -- 
> 1.9.1

Re: [PATCH 4/4] drivers/misc/sgi-gru: fix dereference of ERR_PTR

[Dimitri Sivanich]

On Wed, Sep 02, 2015 at 04:54:58PM +0530, Sudip Mukherjee wrote:
> gru_alloc_gts() can fail and it can return ERR_PTR(errvalue). We should
> not dereference it if it has returned error. And incase it has returned
> error then just downgrade the write lock and exit.
> 

Not sure about this, because gru_load_kernel_context is expected to complete
with a loaded context.  Since most if not all error possibilities are out of
memory conditions, it might be better to loop (with an msleep(1)) and wait
for forward progress, similar to the while loop in this function.

> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>  drivers/misc/sgi-gru/grukservices.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/misc/sgi-gru/grukservices.c b/drivers/misc/sgi-gru/grukservices.c
> index a6fd773..369f7fe 100644
> --- a/drivers/misc/sgi-gru/grukservices.c
> +++ b/drivers/misc/sgi-gru/grukservices.c
> @@ -161,6 +161,8 @@ static void gru_load_kernel_context(struct gru_blade_state *bs, int blade_id)
>  
>  	if (!bs->bs_kgts) {
>  		bs->bs_kgts = gru_alloc_gts(NULL, 0, 0, 0, 0, 0);
> +		if (IS_ERR(bs->bs_kgts))
> +			goto err_gru_alloc_gts;
>  		bs->bs_kgts->ts_user_blade_id = blade_id;
>  	}
>  	kgts = bs->bs_kgts;
> @@ -184,6 +186,7 @@ static void gru_load_kernel_context(struct gru_blade_state *bs, int blade_id)
>  		bs->kernel_cb = get_gseg_base_address_cb(vaddr, ctxnum, 0);
>  		bs->kernel_dsr = get_gseg_base_address_ds(vaddr, ctxnum, 0);
>  	}
> +err_gru_alloc_gts:
>  	downgrade_write(&bs->bs_kgts_sema);
>  }
>  
> -- 
> 1.9.1

Re: [PATCH 1/4] drivers/misc/sgi-gru: remove unused variable

[Sudip Mukherjee]

On Wed, Sep 02, 2015 at 09:42:04AM -0500, Dimitri Sivanich wrote:
> On Wed, Sep 02, 2015 at 04:54:55PM +0530, Sudip Mukherjee wrote:
> > These variables were only assigned some value and were never used.
> > 
> > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > ---
> > 
<snip>
> > +	int i;
> +
> + 	if ((GRU_NUM_TFM * GRU_CACHE_LINE_BYTES) > (ubufend - ubuf))
> +		return -EFBIG;
Just a minor (or major) doubt.
The function returns GRU_NUM_TFM * GRU_CACHE_LINE_BYTES and the for loop
is also running till GRU_NUM_TFM so I am assuming that the function can
handle buffer till size of GRU_NUM_TFM * GRU_CACHE_LINE_BYTES. So the
error -EFBIG should occur when the buffer is more than this. Then
shouldn't it be:
if ((GRU_NUM_TFM * GRU_CACHE_LINE_BYTES) < (ubufend - ubuf))
	return -EFBIG;

or i am wrong in interpreting it?

regards
sudip

Re: [PATCH 1/4] drivers/misc/sgi-gru: remove unused variable

[Dimitri Sivanich]

On Thu, Sep 03, 2015 at 10:10:12AM +0530, Sudip Mukherjee wrote:
> On Wed, Sep 02, 2015 at 09:42:04AM -0500, Dimitri Sivanich wrote:
> > On Wed, Sep 02, 2015 at 04:54:55PM +0530, Sudip Mukherjee wrote:
> > > These variables were only assigned some value and were never used.
> > > 
> > > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > > ---
> > > 
> <snip>
> > > +	int i;
> > +
> > + 	if ((GRU_NUM_TFM * GRU_CACHE_LINE_BYTES) > (ubufend - ubuf))
> > +		return -EFBIG;
> Just a minor (or major) doubt.
> The function returns GRU_NUM_TFM * GRU_CACHE_LINE_BYTES and the for loop
> is also running till GRU_NUM_TFM so I am assuming that the function can
> handle buffer till size of GRU_NUM_TFM * GRU_CACHE_LINE_BYTES. So the
> error -EFBIG should occur when the buffer is more than this. Then
> shouldn't it be:
> if ((GRU_NUM_TFM * GRU_CACHE_LINE_BYTES) < (ubufend - ubuf))
> 	return -EFBIG;
> 
> or i am wrong in interpreting it?
>
The likely intent was to return an error when the buffer isn't big enough
to hold GRU_NUM_TFM * GRU_CACHE_LINE_BYTES.