[PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static

[PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static

[Colin King]

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

Re: [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static

[Jani Nikula]

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

Re: [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static

[Eric Anholt]

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

Re: [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static

[Dan Carpenter]

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

Re: [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static

[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

Re: [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static

[Eric Anholt]

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

Re: [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static

[Dan Carpenter]

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

Re: [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static

[Jani Nikula]

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

Re: [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static

[Dan Carpenter]

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

Re: [PATCH][drm-next] drm/pl111: make structure pl111_display_funcs static

[Daniel Vetter]

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