WTF: patch "[PATCH] drm/mgag200: Remove declaration of mgag200_mmap() from header" was seriously submitted to be applied to the 5.8-stable tree?

PATCH [email protected] 7 participants 11 messages Aug 7, 2020 – Aug 11, 2020

Subsystems (4)

Stable branch DRM drivers DRM misc DRM driver for mga G200 graphics chips

lore

Share & tools

lore

[email protected]

author maintainer

2020-08-07 13:30 UTC #0 lore raw

The patch below was submitted to be applied to the 5.8-stable tree.

I fail to see how this patch meets the stable kernel rules as found at Documentation/process/stable-kernel-rules.rst.

I could be totally wrong, and if so, please respond to <[email protected]> and let me know why this patch should be applied. Otherwise, it is now dropped from my patch queues, never to be seen again.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 1d8d42ba365101fa68d210c0e2ed2bc9582fda6c Mon Sep 17 00:00:00 2001
From: Thomas Zimmermann <[email protected]>
Date: Fri, 5 Jun 2020 15:57:50 +0200
Subject: [PATCH] drm/mgag200: Remove declaration of mgag200_mmap() from header
file
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Commit 94668ac796a5 ("drm/mgag200: Convert mgag200 driver to VRAM MM") removed the implementation of mgag200_mmap(). Also remove the declaration.

Signed-off-by Thomas Zimmermann <[email protected]>

Acked-by Sam Ravnborg <[email protected]>

Fixes 94668ac796a5 ("drm/mgag200: Convert mgag200 driver to VRAM MM")

Cc Gerd Hoffmann <[email protected]>

Cc Dave Airlie <[email protected]>

Cc Krzysztof Kozlowski <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Sam Ravnborg <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc "Noralf Trønnes" <[email protected]>

Cc Armijn Hemel <[email protected]>

Cc Alex Deucher <[email protected]>

Cc Emil Velikov <[email protected]>

Cc <[email protected]> # v5.3+

Link https://patchwork.freedesktop.org/patch/msgid/[email protected]

drivers/gpu/drm/mgag200/mgag200_drv.h +0 -1

index 47df62b1ad29..92b6679029fe 100644
@@ -198,6 +198,5 @@ void mgag200_i2c_destroy(struct mga_i2c_chan *i2c);
 
 int mgag200_mm_init(struct mga_device *mdev);
 void mgag200_mm_fini(struct mga_device *mdev);
-int mgag200_mmap(struct file *filp, struct vm_area_struct *vma);
 
 #endif				/* __MGAG200_DRV_H__ */

Thomas Zimmermann

maintainer

2020-08-07 13:54 UTC #1 lore raw

Am 07.08.20 um 15:30 schrieb [email protected]:

Quoted 9 lines

The patch below was submitted to be applied to the 5.8-stable tree.
I fail to see how this patch meets the stable kernel rules as found at Documentation/process/stable-kernel-rules.rst.
I could be totally wrong, and if so, please respond to <[email protected]> and let me know why this patch should be applied. Otherwise, it is now dropped from my patch queues, never to be seen again.

Sorry for the noise. There's no reason this should go into stable.

Best regards
Thomas

Quoted 48 lines

thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 1d8d42ba365101fa68d210c0e2ed2bc9582fda6c Mon Sep 17 00:00:00 2001
From: Thomas Zimmermann <[email protected]>
Date: Fri, 5 Jun 2020 15:57:50 +0200
Subject: [PATCH] drm/mgag200: Remove declaration of mgag200_mmap() from header
file
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Commit 94668ac796a5 ("drm/mgag200: Convert mgag200 driver to VRAM MM") removed the implementation of mgag200_mmap(). Also remove the declaration.
Signed-off-by Thomas Zimmermann <[email protected]>
Acked-by Sam Ravnborg <[email protected]>
Fixes 94668ac796a5 ("drm/mgag200: Convert mgag200 driver to VRAM MM")
Cc Gerd Hoffmann <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Krzysztof Kozlowski <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Sam Ravnborg <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc "Noralf Trønnes" <[email protected]>
Cc Armijn Hemel <[email protected]>
Cc Alex Deucher <[email protected]>
Cc Emil Velikov <[email protected]>
Cc <[email protected]> # v5.3+
Link https://patchwork.freedesktop.org/patch/msgid/[email protected]
drivers/gpu/drm/mgag200/mgag200_drv.h +0 -1
index 47df62b1ad29..92b6679029fe 100644
@@ -198,6 +198,5 @@ void mgag200_i2c_destroy(struct mga_i2c_chan *i2c);
 
 int mgag200_mm_init(struct mga_device *mdev);
 void mgag200_mm_fini(struct mga_device *mdev);
-int mgag200_mmap(struct file *filp, struct vm_area_struct *vma);
 
 #endif				/* __MGAG200_DRV_H__ */

Signature · 6 lines

Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

Daniel Vetter

2020-08-08 09:14 UTC #2 lore raw

Quoted 9 lines · Thomas Zimmermann

Hi
Am 07.08.20 um 15:30 schrieb [email protected]:
Quoted 5 lines
The patch below was submitted to be applied to the 5.8-stable tree.
I fail to see how this patch meets the stable kernel rules as found at Documentation/process/stable-kernel-rules.rst.

Quoted 6 lines

Quoted 4 lines
I could be totally wrong, and if so, please respond to <[email protected]> and let me know why this patch should be applied. Otherwise, it is now dropped from my patch queues, never to be seen again.
Sorry for the noise. There's no reason this should go into stable.

We have a little script in our maintainer toolbox for bugfixes, which generates the Fixes: line, adds everyone from the original commit to the cc: list and also adds Cc: stable if that sha1 the patch fixes is in a release already.

I guess we trained people a bit too much on using Fixes: tags like that with the tooling, since they often do that for checkpatch stuff and spelling fixes like this here too. I think the autoselect bot also loves Fixes: tags a bit too much for its own good.

Not sure what to do, since telling people to "please sprinkle less
Fixes: tags" doesn't sound great either. I also don't want to tell people to use the maintainer toolbox less, the autogenerated cc: list is generally the right thing to do. Maybe best if the stable team catches the obvious ones before adding them to the stable queue, if you're ok with that Greg?

Also adding dri-devel here in case this becomes a bigger discussion.

Cheers, Daniel

Quoted 61 lines

Best regards
Thomas
Quoted 42 lines
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 1d8d42ba365101fa68d210c0e2ed2bc9582fda6c Mon Sep 17 00:00:00 2001
From: Thomas Zimmermann <[email protected]>
Date: Fri, 5 Jun 2020 15:57:50 +0200
Subject: [PATCH] drm/mgag200: Remove declaration of mgag200_mmap() from header
file
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Commit 94668ac796a5 ("drm/mgag200: Convert mgag200 driver to VRAM MM") removed the implementation of mgag200_mmap(). Also remove the declaration.
Signed-off-by Thomas Zimmermann <[email protected]>
Acked-by Sam Ravnborg <[email protected]>
Fixes 94668ac796a5 ("drm/mgag200: Convert mgag200 driver to VRAM MM")
Cc Gerd Hoffmann <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Krzysztof Kozlowski <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Sam Ravnborg <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc "Noralf Trønnes" <[email protected]>
Cc Armijn Hemel <[email protected]>
Cc Alex Deucher <[email protected]>
Cc Emil Velikov <[email protected]>
Cc <[email protected]> # v5.3+
Link https://patchwork.freedesktop.org/patch/msgid/[email protected]
drivers/gpu/drm/mgag200/mgag200_drv.h +0 -0
index 47df62b1ad29..92b6679029fe 100644
drivers/gpu/drm/mgag200/mgag200_drv.h void mgag200_i2c_destroy(struct mga_i2c_chan *i2c); · line 202
Quoted 6 lines
drivers/gpu/drm/mgag200/mgag200_drv.h +0 -1
@@ -198,6 +198,5 @@ void mgag200_i2c_destroy(struct mga_i2c_chan *i2c);
 
 int mgag200_mm_init(struct mga_device *mdev);
 void mgag200_mm_fini(struct mga_device *mdev);
-int mgag200_mmap(struct file *filp, struct vm_area_struct *vma);
 
 #endif                               /* __MGAG200_DRV_H__ */
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Sam Ravnborg

2020-08-08 09:37 UTC #3 lore raw

Hi Daniel.

Quoted 22 lines · Daniel Vetter

Quoted 9 lines · Thomas Zimmermann
Hi
Am 07.08.20 um 15:30 schrieb [email protected]:
Quoted 5 lines
The patch below was submitted to be applied to the 5.8-stable tree.
I fail to see how this patch meets the stable kernel rules as found at Documentation/process/stable-kernel-rules.rst.
Quoted 6 lines
Quoted 4 lines
I could be totally wrong, and if so, please respond to <[email protected]> and let me know why this patch should be applied. Otherwise, it is now dropped from my patch queues, never to be seen again.
Sorry for the noise. There's no reason this should go into stable.
We have a little script in our maintainer toolbox for bugfixes, which generates the Fixes: line, adds everyone from the original commit to the cc: list and also adds Cc: stable if that sha1 the patch fixes is in a release already.

Quoted 7 lines

I guess we trained people a bit too much on using Fixes: tags like that with the tooling, since they often do that for checkpatch stuff and spelling fixes like this here too. I think the autoselect bot also loves Fixes: tags a bit too much for its own good.
Not sure what to do, since telling people to "please sprinkle less
Fixes: tags" doesn't sound great either.

We know that at lot of the drm people uses "dim fixes". So maybe teach them a litte here?

dim +4 -0

index e4f4d2e..d4fd310 100755
@@ -2428,6 +2428,10 @@ function dim_fixes
 
 	sha1=${1:?$usage}
 
+	echo ""
+	echo "Note: Patch must meet the stable-kernel-rules criterias (Documentation/process/stable-kernel-rules.rst)"
+	echo ""
+
 	cd $DIM_PREFIX/$DIM_REPO
 	echo "Fixes: $(dim_cite $sha1)"

Output would then look like this:

$ dim fixes 1d8d42ba365101fa68d210c0e2ed2bc9582fda6c

Note: Patch must meet the stable-kernel-rules criterias (Documentation/process/stable-kernel-rules.rst)

Fixes 1d8d42ba3651 ("drm/mgag200: Remove declaration of mgag200_mmap() from header file")

Cc Thomas Zimmermann <[email protected]>

Cc Sam Ravnborg <[email protected]>

Cc Gerd Hoffmann <[email protected]>

Cc Dave Airlie <[email protected]>

Cc Krzysztof Kozlowski <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc "Noralf Trønnes" <[email protected]>

Cc Armijn Hemel <[email protected]>

Cc Alex Deucher <[email protected]>

Cc Emil Velikov <[email protected]>

Cc <[email protected]> # v5.3+

Cc Lyude Paul <[email protected]>

No guarantee that people will look up the rules outlined in stable-kernel-rules.rst - but at least a reminder.

Sam

Quoted 77 lines

I also don't want to tell
people to use the maintainer toolbox less, the autogenerated cc: list is generally the right thing to do. Maybe best if the stable team catches the obvious ones before adding them to the stable queue, if you're ok with that Greg?
Also adding dri-devel here in case this becomes a bigger discussion.
Cheers, Daniel
Quoted 61 lines
Best regards
Thomas
Quoted 42 lines
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 1d8d42ba365101fa68d210c0e2ed2bc9582fda6c Mon Sep 17 00:00:00 2001
From: Thomas Zimmermann <[email protected]>
Date: Fri, 5 Jun 2020 15:57:50 +0200
Subject: [PATCH] drm/mgag200: Remove declaration of mgag200_mmap() from header
file
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Commit 94668ac796a5 ("drm/mgag200: Convert mgag200 driver to VRAM MM") removed the implementation of mgag200_mmap(). Also remove the declaration.
Signed-off-by Thomas Zimmermann <[email protected]>
Acked-by Sam Ravnborg <[email protected]>
Fixes 94668ac796a5 ("drm/mgag200: Convert mgag200 driver to VRAM MM")
Cc Gerd Hoffmann <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Krzysztof Kozlowski <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Sam Ravnborg <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc "Noralf Trønnes" <[email protected]>
Cc Armijn Hemel <[email protected]>
Cc Alex Deucher <[email protected]>
Cc Emil Velikov <[email protected]>
Cc <[email protected]> # v5.3+
Link https://patchwork.freedesktop.org/patch/msgid/[email protected]
drivers/gpu/drm/mgag200/mgag200_drv.h +0 -0
index 47df62b1ad29..92b6679029fe 100644
drivers/gpu/drm/mgag200/mgag200_drv.h void mgag200_i2c_destroy(struct mga_i2c_chan *i2c); · line 202
Quoted 6 lines
drivers/gpu/drm/mgag200/mgag200_drv.h +0 -1
@@ -198,6 +198,5 @@ void mgag200_i2c_destroy(struct mga_i2c_chan *i2c);
 
 int mgag200_mm_init(struct mga_device *mdev);
 void mgag200_mm_fini(struct mga_device *mdev);
-int mgag200_mmap(struct file *filp, struct vm_area_struct *vma);
 
 #endif                               /* __MGAG200_DRV_H__ */
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Jani Nikula

2020-08-10 08:20 UTC #4 lore raw

Quoted 41 lines · Sam Ravnborg

Hi Daniel.
Quoted 22 lines · Daniel Vetter
Quoted 9 lines · Thomas Zimmermann
Hi
Am 07.08.20 um 15:30 schrieb [email protected]:
Quoted 5 lines
The patch below was submitted to be applied to the 5.8-stable tree.
I fail to see how this patch meets the stable kernel rules as found at Documentation/process/stable-kernel-rules.rst.
Quoted 6 lines
Quoted 4 lines
I could be totally wrong, and if so, please respond to <[email protected]> and let me know why this patch should be applied. Otherwise, it is now dropped from my patch queues, never to be seen again.
Sorry for the noise. There's no reason this should go into stable.
We have a little script in our maintainer toolbox for bugfixes, which generates the Fixes: line, adds everyone from the original commit to the cc: list and also adds Cc: stable if that sha1 the patch fixes is in a release already.
Quoted 7 lines
I guess we trained people a bit too much on using Fixes: tags like that with the tooling, since they often do that for checkpatch stuff and spelling fixes like this here too. I think the autoselect bot also loves Fixes: tags a bit too much for its own good.
Not sure what to do, since telling people to "please sprinkle less
Fixes: tags" doesn't sound great either.
We know that at lot of the drm people uses "dim fixes". So maybe teach them a litte here?
dim +0 -0
index e4f4d2e..d4fd310 100755

dim function dim_fixes · line 2434

Quoted 7 lines

dim +4 -0

@@ -2428,6 +2428,10 @@ function dim_fixes
 
 	sha1=${1:?$usage}
 
+	echo ""
+	echo "Note: Patch must meet the stable-kernel-rules criterias (Documentation/process/stable-kernel-rules.rst)"
+	echo ""
+

I don't think this is right, because we also use Fixes: to refer to commits that haven't even been merged upstream yet, i.e. commits that are in the -next feature branches, and the stable kernel rules do not apply.

BR,
Jani.

Quoted 112 lines

dim +0 -0
 	cd $DIM_PREFIX/$DIM_REPO
 	echo "Fixes: $(dim_cite $sha1)"
 
Output would then look like this:
$ dim fixes 1d8d42ba365101fa68d210c0e2ed2bc9582fda6c
Note: Patch must meet the stable-kernel-rules criterias (Documentation/process/stable-kernel-rules.rst)
Fixes 1d8d42ba3651 ("drm/mgag200: Remove declaration of mgag200_mmap() from header file")
Cc Thomas Zimmermann <[email protected]>
Cc Sam Ravnborg <[email protected]>
Cc Gerd Hoffmann <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Krzysztof Kozlowski <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc "Noralf Trønnes" <[email protected]>
Cc Armijn Hemel <[email protected]>
Cc Alex Deucher <[email protected]>
Cc Emil Velikov <[email protected]>
Cc <[email protected]> # v5.3+
Cc Lyude Paul <[email protected]>
No guarantee that people will look up the rules outlined in stable-kernel-rules.rst - but at least a reminder.
Sam
Quoted 60 lines
I also don't want to tell
people to use the maintainer toolbox less, the autogenerated cc: list is generally the right thing to do. Maybe best if the stable team catches the obvious ones before adding them to the stable queue, if you're ok with that Greg?
Also adding dri-devel here in case this becomes a bigger discussion.
Cheers, Daniel
Quoted 50 lines
Best regards
Thomas
Quoted 46 lines
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 1d8d42ba365101fa68d210c0e2ed2bc9582fda6c Mon Sep 17 00:00:00 2001
From: Thomas Zimmermann <[email protected]>
Date: Fri, 5 Jun 2020 15:57:50 +0200
Subject: [PATCH] drm/mgag200: Remove declaration of mgag200_mmap() from header
file
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Commit 94668ac796a5 ("drm/mgag200: Convert mgag200 driver to VRAM MM") removed the implementation of mgag200_mmap(). Also remove the declaration.
Signed-off-by Thomas Zimmermann <[email protected]>
Acked-by Sam Ravnborg <[email protected]>
Fixes 94668ac796a5 ("drm/mgag200: Convert mgag200 driver to VRAM MM")
Cc Gerd Hoffmann <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Krzysztof Kozlowski <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Sam Ravnborg <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc "Noralf Trønnes" <[email protected]>
Cc Armijn Hemel <[email protected]>
Cc Alex Deucher <[email protected]>
Cc Emil Velikov <[email protected]>
Cc <[email protected]> # v5.3+
Link https://patchwork.freedesktop.org/patch/msgid/[email protected]
drivers/gpu/drm/mgag200/mgag200_drv.h +0 -1
index 47df62b1ad29..92b6679029fe 100644
@@ -198,6 +198,5 @@ void mgag200_i2c_destroy(struct mga_i2c_chan *i2c);
 
 int mgag200_mm_init(struct mga_device *mdev);
 void mgag200_mm_fini(struct mga_device *mdev);
-int mgag200_mmap(struct file *filp, struct vm_area_struct *vma);
 
Quoted 17 lines
Quoted 11 lines
drivers/gpu/drm/mgag200/mgag200_drv.h void mgag200_i2c_destroy(struct mga_i2c_chan *i2c); · line 202
Quoted 2 lines
drivers/gpu/drm/mgag200/mgag200_drv.h +0 -0
 #endif                               /* __MGAG200_DRV_H__ */
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Signature · 5 lines

Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Greg KH

author maintainer

2020-08-08 10:25 UTC #5 lore raw

In reply to Daniel Vetter

Quoted 27 lines · Daniel Vetter

Quoted 9 lines · Thomas Zimmermann
Hi
Am 07.08.20 um 15:30 schrieb [email protected]:
Quoted 5 lines
The patch below was submitted to be applied to the 5.8-stable tree.
I fail to see how this patch meets the stable kernel rules as found at Documentation/process/stable-kernel-rules.rst.
Quoted 6 lines
Quoted 4 lines
I could be totally wrong, and if so, please respond to <[email protected]> and let me know why this patch should be applied. Otherwise, it is now dropped from my patch queues, never to be seen again.
Sorry for the noise. There's no reason this should go into stable.
We have a little script in our maintainer toolbox for bugfixes, which generates the Fixes: line, adds everyone from the original commit to the cc: list and also adds Cc: stable if that sha1 the patch fixes is in a release already.
I guess we trained people a bit too much on using Fixes: tags like that with the tooling, since they often do that for checkpatch stuff and spelling fixes like this here too. I think the autoselect bot also loves Fixes: tags a bit too much for its own good.

Quoted 6 lines

Not sure what to do, since telling people to "please sprinkle less
Fixes: tags" doesn't sound great either. I also don't want to tell people to use the maintainer toolbox less, the autogenerated cc: list is generally the right thing to do. Maybe best if the stable team catches the obvious ones before adding them to the stable queue, if you're ok with that Greg?

As I think this is the first time that I've had this problem for a DRM submission, I don't think it's a big issue yet at all, so whatever you are doing today is fine.

I do think that the number of patches submitted for stable for drm-related issues feels very very low given the rate of change and number of overall patches you all submit to the kernel, so if anything, you all should be increasing the number of times you tag stuff for stable, not reducing it :)

thanks,

greg k-h

Daniel Vetter

2020-08-08 11:02 UTC #6 lore raw

Quoted 40 lines · Greg KH

Quoted 27 lines · Daniel Vetter
Quoted 9 lines · Thomas Zimmermann
Hi
Am 07.08.20 um 15:30 schrieb [email protected]:
Quoted 5 lines
The patch below was submitted to be applied to the 5.8-stable tree.
I fail to see how this patch meets the stable kernel rules as found at Documentation/process/stable-kernel-rules.rst.
Quoted 6 lines
Quoted 4 lines
I could be totally wrong, and if so, please respond to <[email protected]> and let me know why this patch should be applied. Otherwise, it is now dropped from my patch queues, never to be seen again.
Sorry for the noise. There's no reason this should go into stable.
We have a little script in our maintainer toolbox for bugfixes, which generates the Fixes: line, adds everyone from the original commit to the cc: list and also adds Cc: stable if that sha1 the patch fixes is in a release already.
I guess we trained people a bit too much on using Fixes: tags like that with the tooling, since they often do that for checkpatch stuff and spelling fixes like this here too. I think the autoselect bot also loves Fixes: tags a bit too much for its own good.
Quoted 6 lines
Not sure what to do, since telling people to "please sprinkle less
Fixes: tags" doesn't sound great either. I also don't want to tell people to use the maintainer toolbox less, the autogenerated cc: list is generally the right thing to do. Maybe best if the stable team catches the obvious ones before adding them to the stable queue, if you're ok with that Greg?
As I think this is the first time that I've had this problem for a DRM submission, I don't think it's a big issue yet at all, so whatever you are doing today is fine.

Quoted 5 lines

I do think that the number of patches submitted for stable for drm-related issues feels very very low given the rate of change and number of overall patches you all submit to the kernel, so if anything, you all should be increasing the number of times you tag stuff for stable, not reducing it :)

Ok, sounds like we should encourage people to use the Fixes: tag and auto-cc tooling more, not less.

I also crunched some quick numbers:
commits with cc: stable in drm/amd: 2.6%
... in drm/i915: 2.5%
... drm overall: 2.3%
drivers/ overall: 3.1%

So from a quick look no big outliers at least, maybe not quite enough
cc: stable from smaller drivers (i915+amd is about 60% of everything in drm). This is for the past year. Compared to drivers/ overall a bit lower, but not drastically so. At least if I didn't screw up my scripting.
-Daniel

Signature · 7 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Greg KH

author maintainer

2020-08-08 11:28 UTC #7 lore raw

Quoted 56 lines · Daniel Vetter

Quoted 40 lines · Greg KH
Quoted 27 lines · Daniel Vetter
Quoted 9 lines · Thomas Zimmermann
Hi
Am 07.08.20 um 15:30 schrieb [email protected]:
Quoted 5 lines
The patch below was submitted to be applied to the 5.8-stable tree.
I fail to see how this patch meets the stable kernel rules as found at Documentation/process/stable-kernel-rules.rst.
Quoted 6 lines
Quoted 4 lines
I could be totally wrong, and if so, please respond to <[email protected]> and let me know why this patch should be applied. Otherwise, it is now dropped from my patch queues, never to be seen again.
Sorry for the noise. There's no reason this should go into stable.
We have a little script in our maintainer toolbox for bugfixes, which generates the Fixes: line, adds everyone from the original commit to the cc: list and also adds Cc: stable if that sha1 the patch fixes is in a release already.
I guess we trained people a bit too much on using Fixes: tags like that with the tooling, since they often do that for checkpatch stuff and spelling fixes like this here too. I think the autoselect bot also loves Fixes: tags a bit too much for its own good.
Quoted 6 lines
Not sure what to do, since telling people to "please sprinkle less
Fixes: tags" doesn't sound great either. I also don't want to tell people to use the maintainer toolbox less, the autogenerated cc: list is generally the right thing to do. Maybe best if the stable team catches the obvious ones before adding them to the stable queue, if you're ok with that Greg?
As I think this is the first time that I've had this problem for a DRM submission, I don't think it's a big issue yet at all, so whatever you are doing today is fine.
Quoted 5 lines
I do think that the number of patches submitted for stable for drm-related issues feels very very low given the rate of change and number of overall patches you all submit to the kernel, so if anything, you all should be increasing the number of times you tag stuff for stable, not reducing it :)
Ok, sounds like we should encourage people to use the Fixes: tag and auto-cc tooling more, not less.
I also crunched some quick numbers:
commits with cc: stable in drm/amd: 2.6%
... in drm/i915: 2.5%
... drm overall: 2.3%
drivers/ overall: 3.1%

Quoted 5 lines

So from a quick look no big outliers at least, maybe not quite enough
cc: stable from smaller drivers (i915+amd is about 60% of everything in drm). This is for the past year. Compared to drivers/ overall a bit lower, but not drastically so. At least if I didn't screw up my scripting.

Seems about right, so on those averages, you have missed about 40-50 patches that should have been cc:ed stable.

However, you are comparing yourself against stuff like drivers/net/ which shouldn't have cc: stable for most stuff (as per the networking workflow), and other subsystems that seem to never want to cc: stable for various reasons (offenders not mentioned to be nice...)

So let's bump that number up a bit, maybe you are missing 100 patches this past year that should have been backported?

Feels like you all could tag more, even if the number is only 40-50 :)

Oh wait, are you sure you don't count the horrid "double commits" where you backport something from your development branch to your "for linus" branch, and have cc: stable on both, so that during the -rc1 merge window I see a ton of commits that are already in the tree? That would inflate your numbers a lot more so your real percentages might be a lot lower...

fun with math.

greg k-h
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter

2020-08-08 15:25 UTC #8 lore raw

Quoted 77 lines · Greg KH

Quoted 56 lines · Daniel Vetter
Quoted 40 lines · Greg KH
Quoted 27 lines · Daniel Vetter
Quoted 9 lines · Thomas Zimmermann
Hi
Am 07.08.20 um 15:30 schrieb [email protected]:
Quoted 5 lines
The patch below was submitted to be applied to the 5.8-stable tree.
I fail to see how this patch meets the stable kernel rules as found at Documentation/process/stable-kernel-rules.rst.
Quoted 6 lines
Quoted 4 lines
I could be totally wrong, and if so, please respond to <[email protected]> and let me know why this patch should be applied. Otherwise, it is now dropped from my patch queues, never to be seen again.
Sorry for the noise. There's no reason this should go into stable.
We have a little script in our maintainer toolbox for bugfixes, which generates the Fixes: line, adds everyone from the original commit to the cc: list and also adds Cc: stable if that sha1 the patch fixes is in a release already.
I guess we trained people a bit too much on using Fixes: tags like that with the tooling, since they often do that for checkpatch stuff and spelling fixes like this here too. I think the autoselect bot also loves Fixes: tags a bit too much for its own good.
Quoted 6 lines
Not sure what to do, since telling people to "please sprinkle less
Fixes: tags" doesn't sound great either. I also don't want to tell people to use the maintainer toolbox less, the autogenerated cc: list is generally the right thing to do. Maybe best if the stable team catches the obvious ones before adding them to the stable queue, if you're ok with that Greg?
As I think this is the first time that I've had this problem for a DRM submission, I don't think it's a big issue yet at all, so whatever you are doing today is fine.
Quoted 5 lines
I do think that the number of patches submitted for stable for drm-related issues feels very very low given the rate of change and number of overall patches you all submit to the kernel, so if anything, you all should be increasing the number of times you tag stuff for stable, not reducing it :)
Ok, sounds like we should encourage people to use the Fixes: tag and auto-cc tooling more, not less.
I also crunched some quick numbers:
commits with cc: stable in drm/amd: 2.6%
... in drm/i915: 2.5%
... drm overall: 2.3%
drivers/ overall: 3.1%
Quoted 5 lines
So from a quick look no big outliers at least, maybe not quite enough
cc: stable from smaller drivers (i915+amd is about 60% of everything in drm). This is for the past year. Compared to drivers/ overall a bit lower, but not drastically so. At least if I didn't screw up my scripting.
Seems about right, so on those averages, you have missed about 40-50 patches that should have been cc:ed stable.
However, you are comparing yourself against stuff like drivers/net/ which shouldn't have cc: stable for most stuff (as per the networking workflow), and other subsystems that seem to never want to cc: stable for various reasons (offenders not mentioned to be nice...)
So let's bump that number up a bit, maybe you are missing 100 patches this past year that should have been backported?
Feels like you all could tag more, even if the number is only 40-50 :)

Quoted 8 lines

Oh wait, are you sure you don't count the horrid "double commits" where you backport something from your development branch to your "for linus" branch, and have cc: stable on both, so that during the -rc1 merge window I see a ton of commits that are already in the tree? That would inflate your numbers a lot more so your real percentages might be a lot lower...
fun with math.

Even drivers/net has like 1.0% cc: stable or so, but yeah maybe a third cc: stable might be missing overall in drm. The math aint more accurate no matter what, but agrees with your "about 100 patches".

And yeah I didn't take out the cherry-picked ones. Trying to grep for those (yay more fun with math) says there's 37 stable commits I double-counted, leaving 1.4% left over for drm/i915. That seems indeed a bit too low :-/

I guess time to add intel maintainers (kinda not my direct business anymore). -Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Daniel Vetter

2020-08-10 12:49 UTC #9 lore raw

Quoted 91 lines · Daniel Vetter

Quoted 77 lines · Greg KH
Quoted 56 lines · Daniel Vetter
Quoted 40 lines · Greg KH
Quoted 27 lines · Daniel Vetter
Quoted 9 lines · Thomas Zimmermann
Hi
Am 07.08.20 um 15:30 schrieb [email protected]:
Quoted 5 lines
The patch below was submitted to be applied to the 5.8-stable tree.
I fail to see how this patch meets the stable kernel rules as found at Documentation/process/stable-kernel-rules.rst.
Quoted 6 lines
Quoted 4 lines
I could be totally wrong, and if so, please respond to <[email protected]> and let me know why this patch should be applied. Otherwise, it is now dropped from my patch queues, never to be seen again.
Sorry for the noise. There's no reason this should go into stable.
We have a little script in our maintainer toolbox for bugfixes, which generates the Fixes: line, adds everyone from the original commit to the cc: list and also adds Cc: stable if that sha1 the patch fixes is in a release already.
I guess we trained people a bit too much on using Fixes: tags like that with the tooling, since they often do that for checkpatch stuff and spelling fixes like this here too. I think the autoselect bot also loves Fixes: tags a bit too much for its own good.
Quoted 6 lines
Not sure what to do, since telling people to "please sprinkle less
Fixes: tags" doesn't sound great either. I also don't want to tell people to use the maintainer toolbox less, the autogenerated cc: list is generally the right thing to do. Maybe best if the stable team catches the obvious ones before adding them to the stable queue, if you're ok with that Greg?
As I think this is the first time that I've had this problem for a DRM submission, I don't think it's a big issue yet at all, so whatever you are doing today is fine.
Quoted 5 lines
I do think that the number of patches submitted for stable for drm-related issues feels very very low given the rate of change and number of overall patches you all submit to the kernel, so if anything, you all should be increasing the number of times you tag stuff for stable, not reducing it :)
Ok, sounds like we should encourage people to use the Fixes: tag and auto-cc tooling more, not less.
I also crunched some quick numbers:
commits with cc: stable in drm/amd: 2.6%
... in drm/i915: 2.5%
... drm overall: 2.3%
drivers/ overall: 3.1%
Quoted 5 lines
So from a quick look no big outliers at least, maybe not quite enough
cc: stable from smaller drivers (i915+amd is about 60% of everything in drm). This is for the past year. Compared to drivers/ overall a bit lower, but not drastically so. At least if I didn't screw up my scripting.
Seems about right, so on those averages, you have missed about 40-50 patches that should have been cc:ed stable.
However, you are comparing yourself against stuff like drivers/net/ which shouldn't have cc: stable for most stuff (as per the networking workflow), and other subsystems that seem to never want to cc: stable for various reasons (offenders not mentioned to be nice...)
So let's bump that number up a bit, maybe you are missing 100 patches this past year that should have been backported?
Feels like you all could tag more, even if the number is only 40-50 :)
Quoted 8 lines
Oh wait, are you sure you don't count the horrid "double commits" where you backport something from your development branch to your "for linus" branch, and have cc: stable on both, so that during the -rc1 merge window I see a ton of commits that are already in the tree? That would inflate your numbers a lot more so your real percentages might be a lot lower...
fun with math.
Even drivers/net has like 1.0% cc: stable or so, but yeah maybe a third cc: stable might be missing overall in drm. The math aint more accurate no matter what, but agrees with your "about 100 patches".

Quoted 6 lines

And yeah I didn't take out the cherry-picked ones. Trying to grep for those (yay more fun with math) says there's 37 stable commits I double-counted, leaving 1.4% left over for drm/i915. That seems indeed a bit too low :-/
I guess time to add intel maintainers (kinda not my direct business anymore).

So for comparison I also looked at mesa3d, which at least compared to drivers/gpu is very similar:
- 3 months release cycle, 1 month -rc
- very low level C codebase dealing with gpu nonsense
- same Cc: stable process, shamelessly copied from the kernel
- roughly same review process, but recently switched from patch bombs on m-l to gitlab merge requests (but still pretty similar flow with detailed per-commit review)

It has a 0.9% stable ratio over the past year.

The really big difference is that mesa3d CI is really, really good. Like we run a ton of unit tests, sw rendering tests and then a bunch of hw platforms running validation suits. All pre-merge, i.e. before the patches are even reviewed in detail. And there's a bot used for merging, to make sure you're patches pass, or they don't go in.

tldr; roughly the same, except a CI that's a few orders of magnitude better than what drm/i915 has (especially wrt sporadic issues). Which I think is still lots better than what any other drm driver has (but it does help the subsytem overall with catching lots of issues in helpers an core code).

So maybe the lower cc: stable is because we catch more crap before it even lands ... no idea really, and no human can go and quickly review 10k patches for why there's fewer cc: stable.

Cheers, Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Vivi, Rodrigo

2020-08-11 14:14 UTC #10 lore raw

Quoted 126 lines · Daniel Vetter

Quoted 91 lines · Daniel Vetter
Quoted 77 lines · Greg KH
Quoted 56 lines · Daniel Vetter
Quoted 40 lines · Greg KH
Quoted 27 lines · Daniel Vetter
Quoted 9 lines · Thomas Zimmermann
Hi
Am 07.08.20 um 15:30 schrieb [email protected]:
Quoted 5 lines
The patch below was submitted to be applied to the 5.8-stable tree.
I fail to see how this patch meets the stable kernel rules as found at Documentation/process/stable-kernel-rules.rst.
Quoted 6 lines
Quoted 4 lines
I could be totally wrong, and if so, please respond to <[email protected]> and let me know why this patch should be applied. Otherwise, it is now dropped from my patch queues, never to be seen again.
Sorry for the noise. There's no reason this should go into stable.
We have a little script in our maintainer toolbox for bugfixes, which generates the Fixes: line, adds everyone from the original commit to the cc: list and also adds Cc: stable if that sha1 the patch fixes is in a release already.
I guess we trained people a bit too much on using Fixes: tags like that with the tooling, since they often do that for checkpatch stuff and spelling fixes like this here too. I think the autoselect bot also loves Fixes: tags a bit too much for its own good.
Quoted 6 lines
Not sure what to do, since telling people to "please sprinkle less
Fixes: tags" doesn't sound great either. I also don't want to tell people to use the maintainer toolbox less, the autogenerated cc: list is generally the right thing to do. Maybe best if the stable team catches the obvious ones before adding them to the stable queue, if you're ok with that Greg?
As I think this is the first time that I've had this problem for a DRM submission, I don't think it's a big issue yet at all, so whatever you are doing today is fine.
Quoted 5 lines
I do think that the number of patches submitted for stable for drm-related issues feels very very low given the rate of change and number of overall patches you all submit to the kernel, so if anything, you all should be increasing the number of times you tag stuff for stable, not reducing it :)
Ok, sounds like we should encourage people to use the Fixes: tag and auto-cc tooling more, not less.
I also crunched some quick numbers:
commits with cc: stable in drm/amd: 2.6%
... in drm/i915: 2.5%
... drm overall: 2.3%
drivers/ overall: 3.1%
Quoted 5 lines
So from a quick look no big outliers at least, maybe not quite enough
cc: stable from smaller drivers (i915+amd is about 60% of everything in drm). This is for the past year. Compared to drivers/ overall a bit lower, but not drastically so. At least if I didn't screw up my scripting.
Seems about right, so on those averages, you have missed about 40-50 patches that should have been cc:ed stable.
However, you are comparing yourself against stuff like drivers/net/ which shouldn't have cc: stable for most stuff (as per the networking workflow), and other subsystems that seem to never want to cc: stable for various reasons (offenders not mentioned to be nice...)
So let's bump that number up a bit, maybe you are missing 100 patches this past year that should have been backported?
Feels like you all could tag more, even if the number is only 40-50 :)
Quoted 8 lines
Oh wait, are you sure you don't count the horrid "double commits" where you backport something from your development branch to your "for linus" branch, and have cc: stable on both, so that during the -rc1 merge window I see a ton of commits that are already in the tree? That would inflate your numbers a lot more so your real percentages might be a lot lower...
fun with math.
Even drivers/net has like 1.0% cc: stable or so, but yeah maybe a third cc: stable might be missing overall in drm. The math aint more accurate no matter what, but agrees with your "about 100 patches".
Quoted 6 lines
And yeah I didn't take out the cherry-picked ones. Trying to grep for those (yay more fun with math) says there's 37 stable commits I double-counted, leaving 1.4% left over for drm/i915. That seems indeed a bit too low :-/
I guess time to add intel maintainers (kinda not my direct business anymore).
So for comparison I also looked at mesa3d, which at least compared to drivers/gpu is very similar:
- 3 months release cycle, 1 month -rc
- very low level C codebase dealing with gpu nonsense
- same Cc: stable process, shamelessly copied from the kernel
- roughly same review process, but recently switched from patch bombs on m-l to gitlab merge requests (but still pretty similar flow with detailed per-commit review)
It has a 0.9% stable ratio over the past year.
The really big difference is that mesa3d CI is really, really good. Like we run a ton of unit tests, sw rendering tests and then a bunch of hw platforms running validation suits. All pre-merge, i.e. before the patches are even reviewed in detail. And there's a bot used for merging, to make sure you're patches pass, or they don't go in.
tldr; roughly the same, except a CI that's a few orders of magnitude better than what drm/i915 has (especially wrt sporadic issues). Which I think is still lots better than what any other drm driver has (but it does help the subsytem overall with catching lots of issues in helpers an core code).

Quoted 3 lines

So maybe the lower cc: stable is because we catch more crap before it even lands ... no idea really, and no human can go and quickly review 10k patches for why there's fewer cc: stable.

Or maybe because we have a higher rate of code refactor? :/

But yes, let's work to encourage more people of using Fixes; and cc-stable properly. Luckily we already have some tools in place (dim fixes <has>) that our developers are used to. This will dump the right "Fixes: <hash><commit-subject>" line, and appropriated Ccs, including stable when needed.

Quoted 6 lines

Cheers, Daniel
Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch