Re: [PATCH AUTOSEL 4.19 044/167] drm/amdgpu: validate user pitch alignment
On 2019-09-07 4:58 p.m., Alex Deucher wrote: > > The patch shuffling doesn't help, but regardless, the same thing could > happen even with a direct committer tree if someone missed the tag when > committing. True, but in the latter case it would at least be possible to add tags referencing persistent commit hashes regardless of when fix-ups happen, whereas with the former this isn't possible before the original change makes it to Linus or at least Dave. -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH AUTOSEL 4.19 044/167] drm/amdgpu: validate user pitch alignment
On Wed, 2019-09-04 at 08:08 -0400, Sasha Levin wrote: > it's better to get > it right rather than to be done quickly :) That also applies to the initial selection of patches for the stable trees. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH AUTOSEL 4.19 044/167] drm/amdgpu: validate user pitch alignment
On Tue, Sep 3, 2019 at 4:16 PM Daniel Vetter wrote: > > On Tue, Sep 3, 2019 at 10:01 PM Sasha Levin wrote: > > > > On Tue, Sep 03, 2019 at 07:03:47PM +0200, Greg KH wrote: > > >On Tue, Sep 03, 2019 at 06:40:43PM +0200, Michel Dänzer wrote: > > >> On 2019-09-03 6:23 p.m., Sasha Levin wrote: > > >> > From: Yu Zhao > > >> > > > >> > [ Upstream commit 89f23b6efef554766177bf51aa754bce14c3e7da ] > > >> > > >> Hold your horses! > > >> > > >> This commit and c4a32b266da7bb702e60381ca0c35eaddbc89a6c had to be > > >> reverted, as they caused regressions. See commits > > >> 25ec429e86bb790e40387a550f0501d0ac55a47c & > > >> 92b0730eaf2d549fdfb10ecc8b71f34b9f472c12 . > > >> > > >> > > >> This isn't bolstering confidence in how these patches are selected... > > > > > >The patch _itself_ said to be backported to the stable trees from 4.2 > > >and newer. Why wouldn't we be confident in doing this? > > > > > >If the patch doesn't want to be backported, then do not add the cc: > > >stable line to it... > > > > This patch was picked because it has a stable tag, which you presumably > > saw as your Reviewed-by tag is in the patch. This is why it was > > backported; it doesn't take AI to backport patches tagged for stable... > > > > The revert of this patch, however: > > > > 1. Didn't have a stable tag. > > 2. Didn't have a "Fixes:" tag. > > 3. Didn't have the usual "the reverts commit ..." string added by git > > when one does a revert. > > > > Which is why we still kick patches for review, even though they had a > > stable tag, just so people could take a look and confirm we're not > > missing anything - like we did here. > > > > I'm not sure what you expected me to do differently here. > > Yeah this looks like fail on the revert side, they need to reference > the reverted commit somehow ... > > Alex, why got this dropped? Is this more fallout from the back > shuffling you're doing between your internal branches behind the > firewall, and the public history? The behind the firewall comments are not really helpful. There aren't any "behind the firewall" trees. Everything is mirrored in public. Yes it is annoying that we don't have a direct committer tree, but the only shuffling is between public trees. The problem is 90% of our customers want packaged out of tree drivers rather than in tree drivers because they are using an old distro or a custom distro or something else so we have to do this dance. I realize there are other dances we could do to solve this problem, but they all have their own set of costs and this is what we have now. The patch shuffling doesn't help, but regardless, the same thing could happen even with a direct committer tree if someone missed the tag when committing. Alex > > Also adding Dave Airlie. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH AUTOSEL 4.19 044/167] drm/amdgpu: validate user pitch alignment
On 2019-09-04 2:08 p.m., Sasha Levin wrote: > > FWIW, I've added another test to my scripts to try and catch these cases > (Revert "%s"). It'll slow down the scripts a bit but it's better to get > it right rather than to be done quickly :) Indeed, thanks! And again sorry for the brouhaha, I just honestly didn't realize before how tricky this case was for the scripts. -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH AUTOSEL 4.19 044/167] drm/amdgpu: validate user pitch alignment
On Wed, Sep 04, 2019 at 10:55:10AM +0200, Michel Dänzer wrote: On 2019-09-03 10:16 p.m., Daniel Vetter wrote: On Tue, Sep 3, 2019 at 10:01 PM Sasha Levin wrote: On Tue, Sep 03, 2019 at 07:03:47PM +0200, Greg KH wrote: On Tue, Sep 03, 2019 at 06:40:43PM +0200, Michel Dänzer wrote: On 2019-09-03 6:23 p.m., Sasha Levin wrote: From: Yu Zhao [ Upstream commit 89f23b6efef554766177bf51aa754bce14c3e7da ] Hold your horses! This commit and c4a32b266da7bb702e60381ca0c35eaddbc89a6c had to be reverted, as they caused regressions. See commits 25ec429e86bb790e40387a550f0501d0ac55a47c & 92b0730eaf2d549fdfb10ecc8b71f34b9f472c12 . This isn't bolstering confidence in how these patches are selected... The patch _itself_ said to be backported to the stable trees from 4.2 and newer. Why wouldn't we be confident in doing this? If the patch doesn't want to be backported, then do not add the cc: stable line to it... This patch was picked because it has a stable tag, which you presumably saw as your Reviewed-by tag is in the patch. This is why it was backported; it doesn't take AI to backport patches tagged for stable... The patches did point to gaps in validation of ioctl parameters passed in by userspace. Unfortunately, they turned out to be too strict, causing valid parameters to spuriously fail. If that wasn't the case, and the patches didn't have stable tags, maybe we'd be having a discussion about why they didn't have the tags now... That's fair, and we're definitely not complaining that these patches had a stable tag, my comment was directed more towards the "This isn't bolstering confidence in how these patches are selected" comment you've made - we basically did what we were told to do and for some reason you got upset :) The revert of this patch, however: 1. Didn't have a stable tag. I guess it didn't occur to me that was necessary, as the patches got reverted within days. Since the original stable tagged patch made it upstream, we're bound to try and select it for stable branches even if there are more changes or reverts later on. We'll try to detect further fixes and reverts, but we're limited by the metadata in the commit message. 2. Didn't have a "Fixes:" tag. 3. Didn't have the usual "the reverts commit ..." string added by git when one does a revert. I suspect that's because there were no stable commit hashes to reference, see below. Which is why we still kick patches for review, even though they had a stable tag, just so people could take a look and confirm we're not missing anything - like we did here. I'm not sure what you expected me to do differently here. Yeah, sorry, I didn't realize it's tricky for scripts to recognize that the patches have been reverted in this case. FWIW, I've added another test to my scripts to try and catch these cases (Revert "%s"). It'll slow down the scripts a bit but it's better to get it right rather than to be done quickly :) -- Thanks, Sasha ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH AUTOSEL 4.19 044/167] drm/amdgpu: validate user pitch alignment
On 2019-09-03 10:16 p.m., Daniel Vetter wrote: > On Tue, Sep 3, 2019 at 10:01 PM Sasha Levin wrote: >> On Tue, Sep 03, 2019 at 07:03:47PM +0200, Greg KH wrote: >>> On Tue, Sep 03, 2019 at 06:40:43PM +0200, Michel Dänzer wrote: On 2019-09-03 6:23 p.m., Sasha Levin wrote: > From: Yu Zhao > > [ Upstream commit 89f23b6efef554766177bf51aa754bce14c3e7da ] Hold your horses! This commit and c4a32b266da7bb702e60381ca0c35eaddbc89a6c had to be reverted, as they caused regressions. See commits 25ec429e86bb790e40387a550f0501d0ac55a47c & 92b0730eaf2d549fdfb10ecc8b71f34b9f472c12 . This isn't bolstering confidence in how these patches are selected... >>> >>> The patch _itself_ said to be backported to the stable trees from 4.2 >>> and newer. Why wouldn't we be confident in doing this? >>> >>> If the patch doesn't want to be backported, then do not add the cc: >>> stable line to it... >> >> This patch was picked because it has a stable tag, which you presumably >> saw as your Reviewed-by tag is in the patch. This is why it was >> backported; it doesn't take AI to backport patches tagged for stable... The patches did point to gaps in validation of ioctl parameters passed in by userspace. Unfortunately, they turned out to be too strict, causing valid parameters to spuriously fail. If that wasn't the case, and the patches didn't have stable tags, maybe we'd be having a discussion about why they didn't have the tags now... >> The revert of this patch, however: >> >> 1. Didn't have a stable tag. I guess it didn't occur to me that was necessary, as the patches got reverted within days. >> 2. Didn't have a "Fixes:" tag. >> 3. Didn't have the usual "the reverts commit ..." string added by git >> when one does a revert. I suspect that's because there were no stable commit hashes to reference, see below. >> Which is why we still kick patches for review, even though they had a >> stable tag, just so people could take a look and confirm we're not >> missing anything - like we did here. >> >> I'm not sure what you expected me to do differently here. Yeah, sorry, I didn't realize it's tricky for scripts to recognize that the patches have been reverted in this case. > Yeah this looks like fail on the revert side, they need to reference > the reverted commit somehow ... > > Alex, why got this dropped? Is this more fallout from the back > shuffling you're doing between your internal branches behind the > firewall, and the public history? I do suspect that was at least a contributing factor. -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH AUTOSEL 4.19 044/167] drm/amdgpu: validate user pitch alignment
On Tue, Sep 3, 2019 at 10:01 PM Sasha Levin wrote: > > On Tue, Sep 03, 2019 at 07:03:47PM +0200, Greg KH wrote: > >On Tue, Sep 03, 2019 at 06:40:43PM +0200, Michel Dänzer wrote: > >> On 2019-09-03 6:23 p.m., Sasha Levin wrote: > >> > From: Yu Zhao > >> > > >> > [ Upstream commit 89f23b6efef554766177bf51aa754bce14c3e7da ] > >> > >> Hold your horses! > >> > >> This commit and c4a32b266da7bb702e60381ca0c35eaddbc89a6c had to be > >> reverted, as they caused regressions. See commits > >> 25ec429e86bb790e40387a550f0501d0ac55a47c & > >> 92b0730eaf2d549fdfb10ecc8b71f34b9f472c12 . > >> > >> > >> This isn't bolstering confidence in how these patches are selected... > > > >The patch _itself_ said to be backported to the stable trees from 4.2 > >and newer. Why wouldn't we be confident in doing this? > > > >If the patch doesn't want to be backported, then do not add the cc: > >stable line to it... > > This patch was picked because it has a stable tag, which you presumably > saw as your Reviewed-by tag is in the patch. This is why it was > backported; it doesn't take AI to backport patches tagged for stable... > > The revert of this patch, however: > > 1. Didn't have a stable tag. > 2. Didn't have a "Fixes:" tag. > 3. Didn't have the usual "the reverts commit ..." string added by git > when one does a revert. > > Which is why we still kick patches for review, even though they had a > stable tag, just so people could take a look and confirm we're not > missing anything - like we did here. > > I'm not sure what you expected me to do differently here. Yeah this looks like fail on the revert side, they need to reference the reverted commit somehow ... Alex, why got this dropped? Is this more fallout from the back shuffling you're doing between your internal branches behind the firewall, and the public history? Also adding Dave Airlie. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH AUTOSEL 4.19 044/167] drm/amdgpu: validate user pitch alignment
On Tue, Sep 03, 2019 at 07:03:47PM +0200, Greg KH wrote: On Tue, Sep 03, 2019 at 06:40:43PM +0200, Michel Dänzer wrote: On 2019-09-03 6:23 p.m., Sasha Levin wrote: > From: Yu Zhao > > [ Upstream commit 89f23b6efef554766177bf51aa754bce14c3e7da ] Hold your horses! This commit and c4a32b266da7bb702e60381ca0c35eaddbc89a6c had to be reverted, as they caused regressions. See commits 25ec429e86bb790e40387a550f0501d0ac55a47c & 92b0730eaf2d549fdfb10ecc8b71f34b9f472c12 . This isn't bolstering confidence in how these patches are selected... The patch _itself_ said to be backported to the stable trees from 4.2 and newer. Why wouldn't we be confident in doing this? If the patch doesn't want to be backported, then do not add the cc: stable line to it... This patch was picked because it has a stable tag, which you presumably saw as your Reviewed-by tag is in the patch. This is why it was backported; it doesn't take AI to backport patches tagged for stable... The revert of this patch, however: 1. Didn't have a stable tag. 2. Didn't have a "Fixes:" tag. 3. Didn't have the usual "the reverts commit ..." string added by git when one does a revert. Which is why we still kick patches for review, even though they had a stable tag, just so people could take a look and confirm we're not missing anything - like we did here. I'm not sure what you expected me to do differently here. -- Thanks, Sasha ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH AUTOSEL 4.19 044/167] drm/amdgpu: validate user pitch alignment
On Tue, Sep 03, 2019 at 06:40:43PM +0200, Michel Dänzer wrote: > On 2019-09-03 6:23 p.m., Sasha Levin wrote: > > From: Yu Zhao > > > > [ Upstream commit 89f23b6efef554766177bf51aa754bce14c3e7da ] > > Hold your horses! > > This commit and c4a32b266da7bb702e60381ca0c35eaddbc89a6c had to be > reverted, as they caused regressions. See commits > 25ec429e86bb790e40387a550f0501d0ac55a47c & > 92b0730eaf2d549fdfb10ecc8b71f34b9f472c12 . > > > This isn't bolstering confidence in how these patches are selected... The patch _itself_ said to be backported to the stable trees from 4.2 and newer. Why wouldn't we be confident in doing this? If the patch doesn't want to be backported, then do not add the cc: stable line to it... greg k-h ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH AUTOSEL 4.19 044/167] drm/amdgpu: validate user pitch alignment
On 2019-09-03 6:23 p.m., Sasha Levin wrote: > From: Yu Zhao > > [ Upstream commit 89f23b6efef554766177bf51aa754bce14c3e7da ] Hold your horses! This commit and c4a32b266da7bb702e60381ca0c35eaddbc89a6c had to be reverted, as they caused regressions. See commits 25ec429e86bb790e40387a550f0501d0ac55a47c & 92b0730eaf2d549fdfb10ecc8b71f34b9f472c12 . This isn't bolstering confidence in how these patches are selected... I'm also a little nervous about others which change values by an order of magnitude. There were cases before where such patches were backported to branches which didn't have other corresponding changes, so they ended up breaking stuff instead of fixing anything. -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
