* [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static @ 2017-05-19 11:02 Colin King 2017-05-19 12:03 ` Jani Nikula 0 siblings, 1 reply; 10+ messages in thread From: Colin King @ 2017-05-19 11:02 UTC (permalink / raw) To: Tom Cooksey, Eric Anholt, David Airlie, dri-devel Cc: kernel-janitors, linux-kernel 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") 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, -- 2.11.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static 2017-05-19 11:02 [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static Colin King @ 2017-05-19 12:03 ` Jani Nikula 2017-05-19 18:19 ` Eric Anholt 2017-05-19 19:47 ` Dan Carpenter 0 siblings, 2 replies; 10+ messages in thread From: Jani Nikula @ 2017-05-19 12:03 UTC (permalink / raw) To: Colin King, Tom Cooksey, Eric Anholt, David Airlie, dri-devel Cc: kernel-janitors, linux-kernel 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, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static 2017-05-19 12:03 ` Jani Nikula @ 2017-05-19 18:19 ` Eric Anholt 2017-05-19 19:40 ` Dan Carpenter 2017-05-19 19:47 ` Dan Carpenter 1 sibling, 1 reply; 10+ messages in thread From: Eric Anholt @ 2017-05-19 18:19 UTC (permalink / raw) To: Jani Nikula, Colin King, Tom Cooksey, David Airlie, dri-devel Cc: kernel-janitors, linux-kernel [-- Attachment #1: Type: text/plain, Size: 893 bytes --] 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static 2017-05-19 18:19 ` Eric Anholt @ 2017-05-19 19:40 ` Dan Carpenter 0 siblings, 0 replies; 10+ messages in thread From: Dan Carpenter @ 2017-05-19 19:40 UTC (permalink / raw) To: Eric Anholt Cc: Jani Nikula, Colin King, Tom Cooksey, David Airlie, dri-devel, kernel-janitors, linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static 2017-05-19 12:03 ` Jani Nikula 2017-05-19 18:19 ` Eric Anholt @ 2017-05-19 19:47 ` Dan Carpenter 2017-05-19 20:08 ` Eric Anholt 1 sibling, 1 reply; 10+ messages in thread From: Dan Carpenter @ 2017-05-19 19:47 UTC (permalink / raw) To: Jani Nikula Cc: Colin King, Tom Cooksey, Eric Anholt, David Airlie, dri-devel, kernel-janitors, linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static 2017-05-19 19:47 ` Dan Carpenter @ 2017-05-19 20:08 ` Eric Anholt 2017-05-19 20:16 ` Dan Carpenter 0 siblings, 1 reply; 10+ messages in thread From: Eric Anholt @ 2017-05-19 20:08 UTC (permalink / raw) To: Dan Carpenter, Jani Nikula Cc: Colin King, Tom Cooksey, David Airlie, dri-devel, kernel-janitors, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1776 bytes --] 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static 2017-05-19 20:08 ` Eric Anholt @ 2017-05-19 20:16 ` Dan Carpenter 2017-05-23 8:19 ` Jani Nikula 0 siblings, 1 reply; 10+ messages in thread From: Dan Carpenter @ 2017-05-19 20:16 UTC (permalink / raw) To: Eric Anholt Cc: Jani Nikula, Colin King, Tom Cooksey, David Airlie, dri-devel, kernel-janitors, linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static 2017-05-19 20:16 ` Dan Carpenter @ 2017-05-23 8:19 ` Jani Nikula 2017-05-23 8:31 ` Dan Carpenter 0 siblings, 1 reply; 10+ messages in thread From: Jani Nikula @ 2017-05-23 8:19 UTC (permalink / raw) To: Dan Carpenter, Eric Anholt Cc: Colin King, Tom Cooksey, David Airlie, dri-devel, kernel-janitors, linux-kernel 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. -- Jani Nikula, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static 2017-05-23 8:19 ` Jani Nikula @ 2017-05-23 8:31 ` Dan Carpenter 2017-05-23 13:01 ` Daniel Vetter 0 siblings, 1 reply; 10+ messages in thread From: Dan Carpenter @ 2017-05-23 8:31 UTC (permalink / raw) To: Jani Nikula Cc: Eric Anholt, Colin King, Tom Cooksey, David Airlie, dri-devel, kernel-janitors, linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static 2017-05-23 8:31 ` Dan Carpenter @ 2017-05-23 13:01 ` Daniel Vetter 0 siblings, 0 replies; 10+ messages in thread From: Daniel Vetter @ 2017-05-23 13:01 UTC (permalink / raw) To: Dan Carpenter Cc: Jani Nikula, kernel-janitors, linux-kernel, dri-devel, Colin King 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-05-23 13:01 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-19 11:02 [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static Colin King 2017-05-19 12:03 ` Jani Nikula 2017-05-19 18:19 ` Eric Anholt 2017-05-19 19:40 ` Dan Carpenter 2017-05-19 19:47 ` Dan Carpenter 2017-05-19 20:08 ` Eric Anholt 2017-05-19 20:16 ` Dan Carpenter 2017-05-23 8:19 ` Jani Nikula 2017-05-23 8:31 ` Dan Carpenter 2017-05-23 13:01 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).