Message ID | 20170519110203.19417-1-colin.king@canonical.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Fri, 19 May 2017, Colin King <colin.king@canonical.com> wrote: > From: Colin Ian King <colin.king@canonical.com> > > structure pl111_display_funcs can be made static as it does not need to be > in global scope. Fixes sparse warning: > > "warning: symbol 'pl111_display_funcs' was not declared. Should it > be static?" > > Fixes: bed41005e6174d ("drm/pl111: Initial drm/kms driver for pl111") The patch looks good and I appreciate what you're doing, but I question the usefulness of adding Fixes: tags for trivial stuff like this. I'd prefer Fixes: was reserved for actual fixes that should be backported to any kernels that have the commit being fixed. The same applies to many other patches you've sent recently. BR, Jani. > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/gpu/drm/pl111/pl111_display.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c > index 39a5c33bce7d..bd8ff82c2fd9 100644 > --- a/drivers/gpu/drm/pl111/pl111_display.c > +++ b/drivers/gpu/drm/pl111/pl111_display.c > @@ -280,7 +280,7 @@ static int pl111_display_prepare_fb(struct drm_simple_display_pipe *pipe, > return drm_fb_cma_prepare_fb(&pipe->plane, plane_state); > } > > -const struct drm_simple_display_pipe_funcs pl111_display_funcs = { > +static const struct drm_simple_display_pipe_funcs pl111_display_funcs = { > .check = pl111_display_check, > .enable = pl111_display_enable, > .disable = pl111_display_disable,
Jani Nikula <jani.nikula@linux.intel.com> writes: > On Fri, 19 May 2017, Colin King <colin.king@canonical.com> wrote: >> From: Colin Ian King <colin.king@canonical.com> >> >> structure pl111_display_funcs can be made static as it does not need to be >> in global scope. Fixes sparse warning: >> >> "warning: symbol 'pl111_display_funcs' was not declared. Should it >> be static?" >> >> Fixes: bed41005e6174d ("drm/pl111: Initial drm/kms driver for pl111") > > The patch looks good and I appreciate what you're doing, but I question > the usefulness of adding Fixes: tags for trivial stuff like this. I'd > prefer Fixes: was reserved for actual fixes that should be backported to > any kernels that have the commit being fixed. Agreed -- since Fixes implies going to stable, we don't want it on non-stable-candidates like this. Reviewed these two and will push without the tag in a moment.
On Fri, May 19, 2017 at 11:19:03AM -0700, Eric Anholt wrote: > Jani Nikula <jani.nikula@linux.intel.com> writes: > > > On Fri, 19 May 2017, Colin King <colin.king@canonical.com> wrote: > >> From: Colin Ian King <colin.king@canonical.com> > >> > >> structure pl111_display_funcs can be made static as it does not need to be > >> in global scope. Fixes sparse warning: > >> > >> "warning: symbol 'pl111_display_funcs' was not declared. Should it > >> be static?" > >> > >> Fixes: bed41005e6174d ("drm/pl111: Initial drm/kms driver for pl111") > > > > The patch looks good and I appreciate what you're doing, but I question > > the usefulness of adding Fixes: tags for trivial stuff like this. I'd > > prefer Fixes: was reserved for actual fixes that should be backported to > > any kernels that have the commit being fixed. > > Agreed -- since Fixes implies going to stable, we don't want it on > non-stable-candidates like this. Reviewed these two and will push > without the tag in a moment. Fixes does NOT imply that it goes to stable. Only a Cc: <stable@vger.kernel.org> implies that. Fixes is purely informational to show where the bug was introduced. Just today I was using it to see if API changes introduce a bugs that take months to fix. regards, dan carpenter
On Fri, May 19, 2017 at 03:03:31PM +0300, Jani Nikula wrote: > On Fri, 19 May 2017, Colin King <colin.king@canonical.com> wrote: > > From: Colin Ian King <colin.king@canonical.com> > > > > structure pl111_display_funcs can be made static as it does not need to be > > in global scope. Fixes sparse warning: > > > > "warning: symbol 'pl111_display_funcs' was not declared. Should it > > be static?" > > > > Fixes: bed41005e6174d ("drm/pl111: Initial drm/kms driver for pl111") > > The patch looks good and I appreciate what you're doing, but I question > the usefulness of adding Fixes: tags for trivial stuff like this. I'd > prefer Fixes: was reserved for actual fixes that should be backported to > any kernels that have the commit being fixed. > > The same applies to many other patches you've sent recently. > The Fixes tag is so so useful for everything. It should be included in every bugfix. (I am the inventor of the Fixes tag). I told Colin to include the Fixes tag on everything. My review process is partly "How was this bug introduced? How can we prevent it from happening again? Who was the original author and have they reviewed the proposed fix?" So I end up looking up the original commit anyway. It helps me a lot to have the Fixes tag there. The Fixes tag is obviously useful for the stable people as well, but that wasn't really the point. regards, dan carpenter
Dan Carpenter <dan.carpenter@oracle.com> writes: > On Fri, May 19, 2017 at 03:03:31PM +0300, Jani Nikula wrote: >> On Fri, 19 May 2017, Colin King <colin.king@canonical.com> wrote: >> > From: Colin Ian King <colin.king@canonical.com> >> > >> > structure pl111_display_funcs can be made static as it does not need to be >> > in global scope. Fixes sparse warning: >> > >> > "warning: symbol 'pl111_display_funcs' was not declared. Should it >> > be static?" >> > >> > Fixes: bed41005e6174d ("drm/pl111: Initial drm/kms driver for pl111") >> >> The patch looks good and I appreciate what you're doing, but I question >> the usefulness of adding Fixes: tags for trivial stuff like this. I'd >> prefer Fixes: was reserved for actual fixes that should be backported to >> any kernels that have the commit being fixed. >> >> The same applies to many other patches you've sent recently. >> > > The Fixes tag is so so useful for everything. It should be included > in every bugfix. (I am the inventor of the Fixes tag). > > I told Colin to include the Fixes tag on everything. My review process > is partly "How was this bug introduced? How can we prevent it from > happening again? Who was the original author and have they reviewed the > proposed fix?" So I end up looking up the original commit anyway. It > helps me a lot to have the Fixes tag there. > > The Fixes tag is obviously useful for the stable people as well, but > that wasn't really the point. OK, that's definitely not how I've read the Documentation/process/submitting-patches.rst description of the Fixes tag, which talks about bugs found with git bisect and things that should go to -stable. I would not have considered what this patch is changing to be a bug.
On Fri, May 19, 2017 at 01:08:20PM -0700, Eric Anholt wrote: > OK, that's definitely not how I've read the > Documentation/process/submitting-patches.rst description of the Fixes > tag, which talks about bugs found with git bisect and things that should > go to -stable. I would not have considered what this patch is changing > to be a bug. True. I don't consider this a bug either. I wouldn't have included a Fixes tag. I pretty much agree with the submitting-patches.rst except it should probably say to include it on more stuff. Fixes: tags are required for all bugfixes to netdev for example. regards, dan carpenter
On Fri, 19 May 2017, Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Fri, May 19, 2017 at 01:08:20PM -0700, Eric Anholt wrote: >> OK, that's definitely not how I've read the >> Documentation/process/submitting-patches.rst description of the Fixes >> tag, which talks about bugs found with git bisect and things that should >> go to -stable. I would not have considered what this patch is changing >> to be a bug. > > True. I don't consider this a bug either. I wouldn't have included a > Fixes tag. > > I pretty much agree with the submitting-patches.rst except it should > probably say to include it on more stuff. Fixes: tags are required for > all bugfixes to netdev for example. We use Fixes: in drm/i915 to basically indicate that the referenced commit has a bug that actually needs to be fixed, this patch is the fix, and should go wherever the referenced commit goes. Annotating typo fixes and missing static keywords and such is just noise from *our* POV, and need to be filtered out. BR, Jani.
On Tue, May 23, 2017 at 11:19:58AM +0300, Jani Nikula wrote: > On Fri, 19 May 2017, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Fri, May 19, 2017 at 01:08:20PM -0700, Eric Anholt wrote: > >> OK, that's definitely not how I've read the > >> Documentation/process/submitting-patches.rst description of the Fixes > >> tag, which talks about bugs found with git bisect and things that should > >> go to -stable. I would not have considered what this patch is changing > >> to be a bug. > > > > True. I don't consider this a bug either. I wouldn't have included a > > Fixes tag. > > > > I pretty much agree with the submitting-patches.rst except it should > > probably say to include it on more stuff. Fixes: tags are required for > > all bugfixes to netdev for example. > > We use Fixes: in drm/i915 to basically indicate that the referenced > commit has a bug that actually needs to be fixed, this patch is the fix, > and should go wherever the referenced commit goes. Annotating typo fixes > and missing static keywords and such is just noise from *our* POV, and > need to be filtered out. Yes, yes. I agree. Fixes should fix a bug. I'm sorry, I didn't read the original patch carefully, I just saw that people said Fixes meant backporting to -stable. regards, dan carpenter
On Tue, May 23, 2017 at 11:31:22AM +0300, Dan Carpenter wrote: > On Tue, May 23, 2017 at 11:19:58AM +0300, Jani Nikula wrote: > > On Fri, 19 May 2017, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > On Fri, May 19, 2017 at 01:08:20PM -0700, Eric Anholt wrote: > > >> OK, that's definitely not how I've read the > > >> Documentation/process/submitting-patches.rst description of the Fixes > > >> tag, which talks about bugs found with git bisect and things that should > > >> go to -stable. I would not have considered what this patch is changing > > >> to be a bug. > > > > > > True. I don't consider this a bug either. I wouldn't have included a > > > Fixes tag. > > > > > > I pretty much agree with the submitting-patches.rst except it should > > > probably say to include it on more stuff. Fixes: tags are required for > > > all bugfixes to netdev for example. > > > > We use Fixes: in drm/i915 to basically indicate that the referenced > > commit has a bug that actually needs to be fixed, this patch is the fix, > > and should go wherever the referenced commit goes. Annotating typo fixes > > and missing static keywords and such is just noise from *our* POV, and > > need to be filtered out. > > Yes, yes. I agree. Fixes should fix a bug. I'm sorry, I didn't read > the original patch carefully, I just saw that people said Fixes meant > backporting to -stable. Yeah we use Fixes: a lot, also to help all our product teams, who have all varying versions of frankenstein kernels. If they cherry-pick some feature from upstream, they need to know which bugfixes to backport. cc: stable is orthogonal to Fixes:, but Fixes should imo indicate a real bugfix (i.e. if you have the first patch, you want all the patches with Fixes: lines referencing that patch). Unfortunately on the mobile/gfx side there's very few customers who just use a stable release, so we need to be rather dutiful with sprinkling Fixes: tags over everything that fixes bugs (but not more, otherwise there's screaming about backporting too much). -Daniel
diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c index 39a5c33bce7d..bd8ff82c2fd9 100644 --- a/drivers/gpu/drm/pl111/pl111_display.c +++ b/drivers/gpu/drm/pl111/pl111_display.c @@ -280,7 +280,7 @@ static int pl111_display_prepare_fb(struct drm_simple_display_pipe *pipe, return drm_fb_cma_prepare_fb(&pipe->plane, plane_state); } -const struct drm_simple_display_pipe_funcs pl111_display_funcs = { +static const struct drm_simple_display_pipe_funcs pl111_display_funcs = { .check = pl111_display_check, .enable = pl111_display_enable, .disable = pl111_display_disable,