[PATCH 0/4] GPU-DRM-QXL: Fine-tuning for three function implementations

[PATCH 0/4] GPU-DRM-QXL: Fine-tuning for three function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 08:08:08 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (4):
  Use kmalloc_array() in qxl_device_init()
  Move three assignments in qxl_device_init()
  Improve a size determination in qxl_driver_load()
  Adjust checks for null pointers in three functions

 drivers/gpu/drm/qxl/qxl_kms.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

-- 
2.10.0

[PATCH 1/4] GPU-DRM-QXL: Use kmalloc_array() in qxl_device_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 21 Sep 2016 22:26:08 +0200

* A multiplication for the size determination of a memory allocation
  indicated that an array data structure should be processed.
  Thus use the corresponding function "kmalloc_array".

  This issue was detected by using the Coccinelle software.

* Replace the specification of a data structure by a pointer dereference
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

* Add a bit of exception handling for a detected null pointer.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/qxl/qxl_kms.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index e642242..76852f1 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -216,10 +216,11 @@ static int qxl_device_init(struct qxl_device *qdev,
 	qdev->slot_id_bits = qdev->rom->slot_id_bits;
 	qdev->va_slot_mask =
 		(~(uint64_t)0) >> (qdev->slot_id_bits + qdev->slot_gen_bits);
-
-	qdev->mem_slots =
-		kmalloc(qdev->n_mem_slots * sizeof(struct qxl_memslot),
-			GFP_KERNEL);
+	qdev->mem_slots = kmalloc_array(qdev->n_mem_slots,
+					sizeof(*qdev->mem_slots),
+					GFP_KERNEL);
+	if (!qdev->mem_slots)
+		return -ENOMEM;
 
 	idr_init(&qdev->release_idr);
 	spin_lock_init(&qdev->release_idr_lock);
-- 
2.10.0

[PATCH 2/4] GPU-DRM-QXL: Move three assignments in qxl_device_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 21 Sep 2016 22:33:54 +0200

Move the assignments for three data structure members to the end
so that they will only be performed if the desired resource allocations
succeeded by this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/qxl/qxl_kms.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index 76852f1..76780c2 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -212,10 +212,6 @@ static int qxl_device_init(struct qxl_device *qdev,
 	/* TODO - slot initialization should happen on reset. where is our
 	 * reset handler? */
 	qdev->n_mem_slots = qdev->rom->slots_end;
-	qdev->slot_gen_bits = qdev->rom->slot_gen_bits;
-	qdev->slot_id_bits = qdev->rom->slot_id_bits;
-	qdev->va_slot_mask =
-		(~(uint64_t)0) >> (qdev->slot_id_bits + qdev->slot_gen_bits);
 	qdev->mem_slots = kmalloc_array(qdev->n_mem_slots,
 					sizeof(*qdev->mem_slots),
 					GFP_KERNEL);
@@ -260,7 +256,10 @@ static int qxl_device_init(struct qxl_device *qdev,
 
 
 	INIT_WORK(&qdev->gc_work, qxl_gc_work);
-
+	qdev->slot_gen_bits = qdev->rom->slot_gen_bits;
+	qdev->slot_id_bits = qdev->rom->slot_id_bits;
+	qdev->va_slot_mask =
+		(~(uint64_t)0) >> (qdev->slot_id_bits + qdev->slot_gen_bits);
 	return 0;
 }
 
-- 
2.10.0

Re: [PATCH 2/4] GPU-DRM-QXL: Move three assignments in qxl_device_init()

[Dan Carpenter]

Guys, please stop accepting patches from Markus!

Markus, you always introduce bugs.  I have asked you over and over to
stop sending "cleanup patches" because you are not careful.  If you
restricted yourself to fixing bugs only then you would maybe fix more
bugs than you introduce but as it you are making the kernel worse.

regards
dan carepnter

Re: GPU-DRM-QXL: Move three assignments in qxl_device_init()

[SF Markus Elfring]

> Guys, please stop accepting patches from Markus!

I would appreciate a bit more explanation for this request.


> Markus, you always introduce bugs.

I find the wording "always" exaggerated.

It can also happen that I make another programming mistake occasionally.


> I have asked you over and over to stop sending "cleanup patches"
> because you are not careful.

Why would my update suggestion be inappropriate here?


> If you restricted yourself to fixing bugs only then you would maybe fix more
> bugs than you introduce but as it you are making the kernel worse.

Would you like to discuss the statistics for my failure (or success) rate
a bit more so that involved issues can be clarified in a constructive way?

Regards,
Markus

Re: GPU-DRM-QXL: Move three assignments in qxl_device_init()

[Gerd Hoffmann]

On Do, 2016-09-22 at 15:11 +0200, SF Markus Elfring wrote:
> > Guys, please stop accepting patches from Markus!
> 
> I would appreciate a bit more explanation for this request.

For starters make sure the patches land actually on the list.
Only the cover letter arrived here.

cheers,
  Gerd

Re: GPU-DRM-QXL: Move three assignments in qxl_device_init()

[SF Markus Elfring]

> For starters make sure the patches land actually on the list.

How do you think about to take another look at this update suggestion
also by the usual archive interfaces?

* https://patchwork.kernel.org/patch/9344521/

* https://lkml.kernel.org/r/<f7eb26ad-39be-2918-627b-5f4981d07808@users.sourceforge.net>


> Only the cover letter arrived here.

Would you like to clarify any more technical difficulties around
the desired message exchange?

Regards,
Markus

Re: GPU-DRM-QXL: Move three assignments in qxl_device_init()

[Gerd Hoffmann]

On Do, 2016-09-22 at 19:16 +0200, SF Markus Elfring wrote:
> > For starters make sure the patches land actually on the list.
> 
> How do you think about to take another look at this update suggestion
> also by the usual archive interfaces?
> 
> * https://patchwork.kernel.org/patch/9344521/
> 
> * https://lkml.kernel.org/r/<f7eb26ad-39be-2918-627b-5f4981d07808@users.sourceforge.net>

I fail to see the point in this change.  On init failure qdev will be
released anyway, and whenever the fields are initialized or not when
kfree() is called doesn't matter at all.

> > Only the cover letter arrived here.
> 
> Would you like to clarify any more technical difficulties around
> the desired message exchange?

Hmm, not sure what happened.  According to the kernel.org link the patch
has been sent to dri-devel (where I'm subscribed), but the patch never
landed in by mailbox for some reason.

cheers,
  Gerd

Re: GPU-DRM-QXL: Move three assignments in qxl_device_init()

[Dan Carpenter]

On Thu, Sep 22, 2016 at 03:11:25PM +0200, SF Markus Elfring wrote:
> > If you restricted yourself to fixing bugs only then you would maybe fix more
> > bugs than you introduce but as it you are making the kernel worse.
> 
> Would you like to discuss the statistics for my failure (or success) rate
> a bit more so that involved issues can be clarified in a constructive way?

It should be that you target 20 bug fixes for each new regression that
you add.

Since you are just sending clean ups, every bug you introduce sets us
further and further back.  There is no hope for improving the kernel
because you are not even trying to fix 20 bugs, only introducing them.

Once you fix 20 bugs, then you will be even and you can start sending
cleanups again.  This is fair.

regards,
dan carpenter

Re: GPU-DRM-QXL: Move three assignments in qxl_device_init()

[Sean Paul]

On Thu, Sep 22, 2016 at 1:24 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Thu, Sep 22, 2016 at 03:11:25PM +0200, SF Markus Elfring wrote:
>> > If you restricted yourself to fixing bugs only then you would maybe fix more
>> > bugs than you introduce but as it you are making the kernel worse.
>>
>> Would you like to discuss the statistics for my failure (or success) rate
>> a bit more so that involved issues can be clarified in a constructive way?
>
> It should be that you target 20 bug fixes for each new regression that
> you add.
>
> Since you are just sending clean ups, every bug you introduce sets us
> further and further back.  There is no hope for improving the kernel
> because you are not even trying to fix 20 bugs, only introducing them.
>
> Once you fix 20 bugs, then you will be even and you can start sending
> cleanups again.  This is fair.
>

At the risk of piling on, but hopefully to benefit Markus going forward:

I will refrain from merging any more style/checkpatch/"code cleanup"
patches from Markus until we start getting real, tested, bug fixes.

Sean


> regards,
> dan carpenter
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: GPU-DRM-QXL: Move three assignments in qxl_device_init()

[Dave Airlie]

On 23 September 2016 at 17:25, Sean Paul <seanpaul@chromium.org> wrote:
> On Thu, Sep 22, 2016 at 1:24 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> On Thu, Sep 22, 2016 at 03:11:25PM +0200, SF Markus Elfring wrote:
>>> > If you restricted yourself to fixing bugs only then you would maybe fix more
>>> > bugs than you introduce but as it you are making the kernel worse.
>>>
>>> Would you like to discuss the statistics for my failure (or success) rate
>>> a bit more so that involved issues can be clarified in a constructive way?
>>
>> It should be that you target 20 bug fixes for each new regression that
>> you add.
>>
>> Since you are just sending clean ups, every bug you introduce sets us
>> further and further back.  There is no hope for improving the kernel
>> because you are not even trying to fix 20 bugs, only introducing them.
>>
>> Once you fix 20 bugs, then you will be even and you can start sending
>> cleanups again.  This is fair.
>>
>
> At the risk of piling on, but hopefully to benefit Markus going forward:
>
> I will refrain from merging any more style/checkpatch/"code cleanup"
> patches from Markus until we start getting real, tested, bug fixes.

I'd prefer if everyone on dri-devel just ignored Markus at this stage,

If you are going to pick up his patches, please spend time making sure they
are correct and tested, as he doesn't seem to.

Markus, please contact the list in advance in future before posting a bunch
of patches that don't fix any problems.

Dave.

Re: GPU-DRM-QXL: Move three assignments in qxl_device_init()

[SF Markus Elfring]

> Markus, please contact the list in advance in future before posting a bunch
> of patches that don't fix any problems.

I am trying to improve various open issues also in Linux source files.

Unfortunately, some of the proposed changes might not fit to your software
development attention at the moment.
How are the chances that corresponding change acceptance will evolve a bit more
after a while?

Regards,
Markus

Re: GPU-DRM-QXL: Move three assignments in qxl_device_init()

[Emil Velikov]

On 23 September 2016 at 09:50, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> Markus, please contact the list in advance in future before posting a bunch
>> of patches that don't fix any problems.
>
> I am trying to improve various open issues also in Linux source files.
>
That the fact that you see issues (in these particular cases) while
others do not indicates that the commit summary could be explained
better.

A good commit summary should provide enough information to do that and
make people _want_ the patch. From my limited experience through your
patches (just skimmed a few) you seems to be describing what the patch
does as opposed to why it does it and why should one find it
interesting/wanted.

You might want to read through the following [1] [2] and many others
that exist out there.

-Emil

[1] http://chris.beams.io/posts/git-commit/
[2] http://who-t.blogspot.fi/2009/12/on-commit-messages.html

Re: GPU-DRM-QXL: Move three assignments in qxl_device_init()

[SF Markus Elfring]

>> I am trying to improve various open issues also in Linux source files.
>>
> That the fact that you see issues (in these particular cases) while
> others do not

I guess that the discussed "story" affects more challenges in communication
and different opinions about where to invest software development attention.


> indicates that the commit summary could be explained better.

I imagine that there are a few improvement possibilities left over.


> A good commit summary should provide enough information to do that

This advice is generally fine.


> and make people _want_ the patch.

I guess that this expectation can become a research topic for some knowledge fields,
can't it?

There are update suggestion where the probability for acceptance is higher
than for others.

Some maintainers have got their own difficulties with changes when they categorise
them as "ordinary clean-up".


> From my limited experience through your patches (just skimmed a few)

Thanks for your general interest.


> you seems to be describing what the patch does

My collection of update suggestions is evolving over some source code search patterns.

I find that this approach fits to the recommended imperative wording style
according to document "SubmittingPatches", doesn't it?

I dared also some deviations or variations already.


> as opposed to why it does it and why should one find it interesting/wanted.

I am trying to express also this information to some degree.

Regards,
Markus

Re: GPU-DRM-QXL: Move three assignments in qxl_device_init()

[SF Markus Elfring]

> I will refrain from merging any more style/checkpatch/"code cleanup"
> patches from Markus until we start getting real, tested, bug fixes.

Can such a kind of feedback be also interpreted in the way that you insist
to keep some weaknesses which I tried to point in the Linux source code out
for another while?

How do you think about to clarify your ranking for various update candidates
in this software?

Regards,
Markus

Re: GPU-DRM-QXL: Move three assignments in qxl_device_init()

[SF Markus Elfring]

>> Would you like to discuss the statistics for my failure (or success) rate
>> a bit more so that involved issues can be clarified in a constructive way?
> 
> It should be that you target 20 bug fixes for each new regression
> that you add.

How do you think about to clarify any concrete "regression" a bit more?


> There is no hope for improving the kernel

I have got an other impression. - I am trying to help also for this goal.


> because you are not even trying to fix 20 bugs,

Under which circumstances would you dare to acknowledge once more
that I improved anything for which you care about?


> only introducing them.

It's a pity that you interpret some of my contributions in this way.


> Once you fix 20 bugs, then you will be even and you can start sending
> cleanups again.  This is fair.

How much will the suggested software refactorings influence the kind of
error counter that you prefer so far?

Regards,
Markus

[PATCH 3/4] GPU-DRM-QXL: Improve a size determination in qxl_driver_load()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 21 Sep 2016 22:48:34 +0200

Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/qxl/qxl_kms.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index 76780c2..f8f0261 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -306,7 +306,7 @@ int qxl_driver_load(struct drm_device *dev, unsigned long flags)
 	struct qxl_device *qdev;
 	int r;
 
-	qdev = kzalloc(sizeof(struct qxl_device), GFP_KERNEL);
+	qdev = kzalloc(sizeof(*qdev), GFP_KERNEL);
 	if (qdev == NULL)
 		return -ENOMEM;
 
-- 
2.10.0

[PATCH 4/4] GPU-DRM-QXL: Adjust checks for null pointers in three functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 08:00:08 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The script "checkpatch.pl" can point information out like the following.

Comparison to NULL could be written !…

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/qxl/qxl_kms.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index f8f0261..f594430 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -149,7 +149,7 @@ static int qxl_device_init(struct qxl_device *qdev,
 			io_mapping_create_wc(qdev->surfaceram_base,
 					     qdev->surfaceram_size);
 	}
-	if (qdev->surface_mapping == NULL) {
+	if (!qdev->surface_mapping) {
 		/* 64bit surface bar not present (or mapping failed) */
 		sb = 1;
 		qdev->surfaceram_base = pci_resource_start(pdev, sb);
@@ -288,7 +288,7 @@ int qxl_driver_unload(struct drm_device *dev)
 {
 	struct qxl_device *qdev = dev->dev_private;
 
-	if (qdev == NULL)
+	if (!qdev)
 		return 0;
 
 	drm_vblank_cleanup(dev);
@@ -307,7 +307,7 @@ int qxl_driver_load(struct drm_device *dev, unsigned long flags)
 	int r;
 
 	qdev = kzalloc(sizeof(*qdev), GFP_KERNEL);
-	if (qdev == NULL)
+	if (!qdev)
 		return -ENOMEM;
 
 	dev->dev_private = qdev;
-- 
2.10.0