Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1488568 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] GPU-DRM-QXL: Fine-tuning for three function  implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-22 08:30 +0200
    [PATCH 2/4] GPU-DRM-QXL: Move three assignments in qxl_device_init() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-22 08:30 +0200
      Re: [PATCH 2/4] GPU-DRM-QXL: Move three assignments in  qxl_device_init() Dan Carpenter <dan.carpenter@oracle.com> - 2016-09-22 12:20 +0200
        Re: GPU-DRM-QXL: Move three assignments in qxl_device_init() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-22 15:20 +0200
          Re: GPU-DRM-QXL: Move three assignments in qxl_device_init() Gerd Hoffmann <kraxel@redhat.com> - 2016-09-22 17:50 +0200
            Re: GPU-DRM-QXL: Move three assignments in qxl_device_init() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-22 19:20 +0200
              Re: GPU-DRM-QXL: Move three assignments in qxl_device_init() Gerd Hoffmann <kraxel@redhat.com> - 2016-09-22 22:30 +0200
          Re: GPU-DRM-QXL: Move three assignments in qxl_device_init() Dan Carpenter <dan.carpenter@oracle.com> - 2016-09-22 22:30 +0200
            Re: GPU-DRM-QXL: Move three assignments in qxl_device_init() Sean Paul <seanpaul@chromium.org> - 2016-09-23 09:30 +0200
              Re: GPU-DRM-QXL: Move three assignments in qxl_device_init() Dave Airlie <airlied@gmail.com> - 2016-09-23 10:00 +0200
                Re: GPU-DRM-QXL: Move three assignments in qxl_device_init() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-23 11:00 +0200
              Re: GPU-DRM-QXL: Move three assignments in qxl_device_init() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-23 10:50 +0200
            Re: GPU-DRM-QXL: Move three assignments in qxl_device_init() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-23 10:40 +0200
    [PATCH 3/4] GPU-DRM-QXL: Improve a size determination in  qxl_driver_load() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-22 08:30 +0200
    [PATCH 4/4] GPU-DRM-QXL: Adjust checks for null pointers in three  functions SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-22 08:30 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

> 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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

> 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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

> 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

[toc] | [prev] | [next] | [standalone]

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

> 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

[toc] | [prev] | [next] | [standalone]

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

>> 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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web