linux-next: Fixes tags need some work in the dma-mapping-fixes tree

linux-next: Fixes tags need some work in the dma-mapping-fixes tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 621 bytes --]

Hi Christoph,

In commit

  702e8ed37bed ("arm64/xen: fix xen-swiotlb cache flushing")

Fixes tag

  Fixes: 356da6d0cd ("dma-mapping: bypass indirect calls for dma-direct")

has these problem(s):

  - SHA1 should be at least 12 digits long

In commit

  8218a55b6b91 ("sbitmap: Protect swap_lock from hardirq")

Fixes tag

  Fixes: ab53dcfb3e7b ("sbitmap: Protect swap_lock from hardirq")

has these problem(s):

  - Target SHA1 does not exist

This later patch appears to already be in Linus' tree as commit
fe76fc6aaf53 (also with an incorrect Fixes tag :-()
-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 484 bytes --]

Re: linux-next: Fixes tags need some work in the dma-mapping-fixes tree

[Christoph Hellwig]

On Wed, Jan 23, 2019 at 07:47:47AM +1100, Stephen Rothwell wrote:
> Hi Christoph,
> 
> In commit
> 
>   702e8ed37bed ("arm64/xen: fix xen-swiotlb cache flushing")
> 
> Fixes tag
> 
>   Fixes: 356da6d0cd ("dma-mapping: bypass indirect calls for dma-direct")
> 
> has these problem(s):
> 
>   - SHA1 should be at least 12 digits long

When did we decide on that?  As far as I know it was bumped to 10
a while ago.  12 basically makes the line even more unreadable.

> In commit
> 
>   8218a55b6b91 ("sbitmap: Protect swap_lock from hardirq")
> 
> Fixes tag
> 
>   Fixes: ab53dcfb3e7b ("sbitmap: Protect swap_lock from hardirq")
> 
> has these problem(s):
> 
>   - Target SHA1 does not exist
> 
> This later patch appears to already be in Linus' tree as commit
> fe76fc6aaf53 (also with an incorrect Fixes tag :-()

That commit is not from the dma-mapping tree..

Re: linux-next: Fixes tags need some work in the dma-mapping-fixes tree

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1780 bytes --]

On Wed, Jan 23, 2019 at 07:47:47AM +1100, Stephen Rothwell wrote:
> Hi Christoph,
> 
> In commit
> 
>   702e8ed37bed ("arm64/xen: fix xen-swiotlb cache flushing")

Hi Christoph,

I'm not subscribed to any of the mailing list that the above patch was
reviewed on, so I'm replying here because it is the only reference that
I have.

The above commit breaks the build on 64-bit ARM, like so:

	  CC      drivers/xen/swiotlb-xen.o
	In file included from drivers/xen/swiotlb-xen.c:47:
	arch/arm64/include/asm/xen/page-coherent.h: In function 'xen_dma_map_page':
	arch/arm64/include/asm/xen/page-coherent.h:53:16: error: 'pfn' undeclared (first use in this function)
	  if (pfn_valid(pfn))
			^~~
	arch/arm64/include/asm/xen/page-coherent.h:53:16: note: each undeclared identifier is reported only once for each function it appears in
	arch/arm64/include/asm/xen/page-coherent.h:50:7: warning: unused variable 'local' [-Wunused-variable]
	  bool local = (page_pfn <= dev_pfn) &&
	       ^~~~~

Given that the "local" variable is unused, it seems like maybe a chunk
is missing from the commit and it may have been meant to look like this:

diff --git a/arch/arm64/include/asm/xen/page-coherent.h b/arch/arm64/include/asm/xen/page-coherent.h
index 77e36decc50c..d88e56b90b93 100644
--- a/arch/arm64/include/asm/xen/page-coherent.h
+++ b/arch/arm64/include/asm/xen/page-coherent.h
@@ -50,7 +50,7 @@ static inline void xen_dma_map_page(struct device *hwdev, struct page *page,
  bool local = (page_pfn <= dev_pfn) &&
   (dev_pfn - page_pfn < compound_pages);

- if (pfn_valid(pfn))
+ if (local)
   dma_direct_map_page(hwdev, page, offset, size, dir, attrs);
  else
   __xen_dma_map_page(hwdev, page, dev_addr, offset, size, dir, attrs);

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: linux-next: Fixes tags need some work in the dma-mapping-fixes tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 2396 bytes --]

Hi Christoph,

On Wed, 23 Jan 2019 08:19:33 +0100 Christoph Hellwig <hch@lst.de> wrote:
>
> On Wed, Jan 23, 2019 at 07:47:47AM +1100, Stephen Rothwell wrote:
> >   - SHA1 should be at least 12 digits long  
> 
> When did we decide on that?  As far as I know it was bumped to 10
> a while ago.  12 basically makes the line even more unreadable.

From Documentation//process/submitting-patches.rst:

"If your patch fixes a bug in a specific commit, e.g. you found an issue using
``git bisect``, please use the 'Fixes:' tag with the first 12 characters of
the SHA-1 ID, and the one line summary."

Apparently we already have some clashes for 11 digit abbreviations.
Also, the git-config man page has:

"core.abbrev
           Set the length object names are abbreviated to. If unspecified or
           set to "auto", an appropriate value is computed based on the
           approximate number of packed objects in your repository, which
           hopefully is enough for abbreviated object names to stay unique for
           some time."

and when I set mine to "auto" it produces 12 digit SHA1 abbreviations
in the linux-next tree and in Linus' tree. So there has been some
discussion of suggesting core.abbrev not be set (which is the same as
"auto"), or being set to 13 to give some leeway.

> > In commit
> > 
> >   8218a55b6b91 ("sbitmap: Protect swap_lock from hardirq")
> > 
> > This later patch appears to already be in Linus' tree as commit
> > fe76fc6aaf53 (also with an incorrect Fixes tag :-()  
> 
> That commit is not from the dma-mapping tree..

This is in my linux-next tree today:

$ git log --oneline origin/master..dma-mapping-fixes/for-linus 
702e8ed37bed arm64/xen: fix xen-swiotlb cache flushing
eda14f8977df nvme-pci: fix nvme_setup_irqs()
7c6f88f2fb4b nvmet-tcp: fix uninitialized variable access
8218a55b6b91 sbitmap: Protect swap_lock from hardirq
f5ac6c1e96a9 md: Make bio_alloc_mddev use bio_alloc_bioset
2490a5f5f2f8 block, bfq: fix comments on __bfq_deactivate_entity

origin/master is Linus' tree and dma-mapping-fixes is
git://git.infradead.org/users/hch/dma-mapping.git#for-linus

However, the committer of 8218a55b6b91 is Jens Axboe, so have you
accidentally based your for-linus branch on something from Jens?  That
commit does not appear in any other branch in linux-next.
-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: Fixes tags need some work in the dma-mapping-fixes tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 954 bytes --]

Hi Christoph,

On Wed, 23 Jan 2019 23:09:15 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> On Wed, 23 Jan 2019 08:19:33 +0100 Christoph Hellwig <hch@lst.de> wrote:
> >
> > On Wed, Jan 23, 2019 at 07:47:47AM +1100, Stephen Rothwell wrote:  
> > >   - SHA1 should be at least 12 digits long    
> > 
> > When did we decide on that?  As far as I know it was bumped to 10
> > a while ago.  12 basically makes the line even more unreadable.  
> 
> From Documentation//process/submitting-patches.rst:
> 
> "If your patch fixes a bug in a specific commit, e.g. you found an issue using
> ``git bisect``, please use the 'Fixes:' tag with the first 12 characters of
> the SHA-1 ID, and the one line summary."

That paragraph was added by commit

  8401aa1f59975 ("Documentation/SubmittingPatches: describe the Fixes: tag")

commited by Linus on 2014-06-07 (before it got moved to the .rst file).

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]