Re: [PATCH 00/11] staging: gasket: fixes

[Christoph Hellwig]

On Sun, Oct 14, 2018 at 09:59:16PM -0700, Todd Poynor wrote:
> From: Todd Poynor 
> 
> Various fixes for gasket/apex drivers.

I still can't find any explanation in the staging tree or in your
comments what gasket even is.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v11 07/11] device-mapping: Introduce DMA range map, supplanting dma_pfn_offset

[Christoph Hellwig]

FYI, I've moved it off for-next and into its own dma-ranges branch
for now until we sort out the regressions.  I still hope to bring it
back soon.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v11 07/11] device-mapping: Introduce DMA range map, supplanting dma_pfn_offset

[Christoph Hellwig]

On Mon, Sep 07, 2020 at 05:18:59PM +0200, Nicolas Saenz Julienne wrote:
> Hi Christoph, a small fix to your fixes:

Thanks,

folded into the patch on the dma-ranges branch.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v11 07/11] device-mapping: Introduce DMA range map, supplanting dma_pfn_offset

[Christoph Hellwig]

On Mon, Sep 07, 2020 at 01:40:46PM -0400, Jim Quinlan wrote:
> Thanks for looking into this.  The concern I have with your solution
> is that returning an arbitrarily large offset might overlap with an
> improbable but valid usage.  AFAIK there is nothing that disallows
> mapping a device to anywhere within the 64bit range of PCIe space,
> including up to say 0x.
> 
> As an alternative perhaps changing dma_capable() so that if the
> dma_range_map is present then it validates that both ends of the
> prospective DMA region get "hits" on one of the offset regions in the
> map.  Christoph, if you are okay with this I can quickly post a patch.

We use a dma_addr of all-Fs as an error code, see the definition of
DMA_MAPPING_ERROR.

The rationale of why we think it is safe:  We don't do single byte
mappings, so the last address of the address space can't effectively
be used for anything.

So I think it would be a good fit here.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v11 07/11] device-mapping: Introduce DMA range map, supplanting dma_pfn_offset

[Christoph Hellwig]

On Mon, Sep 07, 2020 at 08:19:43PM +0200, Nicolas Saenz Julienne wrote:
> Indeed, that's why I wasn't all that happy with my solution.
> 
> As an alternative, how about returning '-dev->bus_dma_limit' instead of 0? 
> It's
> always 0 for the devices without bus_dma_regions, and, I think, an always
> unattainable offset for devices that do (I tested it on RPi4 with the 30bit
> limitd mmc controller and it seems to work alright).

No, bus_dma_limit can be set independent of offsets.  We use it e.g.
to limit old x86 VIA PCI bridges to 32-bit addressing.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v11 07/11] device-mapping: Introduce DMA range map, supplanting dma_pfn_offset

[Christoph Hellwig]

FYI, this is what I'd do relative to the patch on the dma-ranges
branch.  In fact realizing this makes me want to refactor things a bit
so that the new code can entirely live in the dma-direct code, but please
test this first:


diff --git a/arch/arm/include/asm/dma-mapping.h 
b/arch/arm/include/asm/dma-mapping.h
index c21893f683b585..072fc42349874d 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -35,21 +35,16 @@ static inline const struct dma_map_ops 
*get_arch_dma_ops(struct bus_type *bus)
 #ifndef __arch_pfn_to_dma
 static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn)
 {
-   if (dev) {
-   phys_addr_t paddr = PFN_PHYS(pfn);
-
-   pfn -= PFN_DOWN(dma_offset_from_phys_addr(dev, paddr));
-   }
-   return (dma_addr_t)__pfn_to_bus(pfn);
+   if (!dev)
+   return (dma_addr_t)__pfn_to_bus(pfn);
+   return translate_phys_to_dma(dev, PFN_PHYS(pfn));
 }
 
 static inline unsigned long dma_to_pfn(struct device *dev, dma_addr_t addr)
 {
-   unsigned long pfn = __bus_to_pfn(addr);
-
-   if (dev)
-   pfn += PFN_DOWN(dma_offset_from_dma_addr(dev, addr));
-   return pfn;
+   if (!dev)
+   return __bus_to_pfn(addr);
+   return PFN_DOWN(translate_dma_to_phys(dev, addr));
 }
 
 static inline void *dma_to_virt(struct device *dev, dma_addr_t addr)
diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h
index 7831ca5b1b5dd6..e624171c4962ad 100644
--- a/include/linux/dma-direct.h
+++ b/include/linux/dma-direct.h
@@ -19,12 +19,16 @@ extern unsigned int zone_dma_bits;
 #else
 static inline dma_addr_t __phys_to_dma(struct device *dev, phys_addr_t paddr)
 {
-   return (dma_addr_t)paddr - dma_offset_from_phys_addr(dev, paddr);
+   if (dev->dma_range_map)
+   return (dma_addr_t)paddr - translate_phys_to_dma(dev, paddr);
+   return (dma_addr_t)paddr;
 }
 
-static inline phys_addr_t __dma_to_phys(struct device *dev, dma_addr_t 
dev_addr)
+static inline phys_addr_t __dma_to_phys(struct device *dev, dma_addr_t 
dma_addr)
 {
-   return (phys_addr_t)dev_addr + dma_offset_from_dma_addr(dev, dev_addr);
+   if (dev->dma_range_map)
+   return translate_dma_to_phys(dev, dma_addr);
+   return (phys_addr_t)dma_addr;
 }
 #endif /* !CONFIG_ARCH_HAS_PHYS_TO_DMA */
 
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 4c4646761afee4..3b1ceebb6f2ad5 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -199,29 +199,28 @@ struct bus_dma_region {
 };
 
 #ifdef CONFIG_HAS_DMA
-static inline u64 dma_offset_from_dma_addr(struct device *dev,
-   dma_addr_t dma_addr)
+static inline dma_addr_t translate_phys_to_dma(struct device *dev,
+   phys_addr_t paddr)
 {
-   const struct bus_dma_region *m = dev->dma_range_map;
+   const struct bus_dma_region *m;
 
-   if (m)
-   for (; m->size; m++)
-   if (dma_addr >= m->dma_start &&
-   dma_addr - m->dma_start < m->size)
-   return m->offset;
-   return 0;
+   for (m = dev->dma_range_map; m->size; m++)
+   if (paddr >= m->cpu_start && paddr - m->cpu_start < m->size)
+   return (dma_addr_t)paddr - m->offset;
+
+   /* make sure dma_capable fails when no translation is available */
+   return DMA_MAPPING_ERROR; 
 }
 
-static inline u64 dma_offset_from_phys_addr(struct device *dev,
-   phys_addr_t paddr)
+static inline phys_addr_t translate_dma_to_phys(struct device *dev,
+   dma_addr_t dma_addr)
 {
-   const struct bus_dma_region *m = dev->dma_range_map;
+   const struct bus_dma_region *m;
+
+   for (m = dev->dma_range_map; m->size; m++)
+   if (dma_addr >= m->dma_start && dma_addr - m->dma_start < 
m->size)
+   return (phys_addr_t)dma_addr + m->offset;
 
-   if (m)
-   for (; m->size; m++)
-   if (paddr >= m->cpu_start &&
-   paddr - m->cpu_start < m->size)
-   return m->offset;
return 0;
 }
 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v11 07/11] device-mapping: Introduce DMA range map, supplanting dma_pfn_offset

[Christoph Hellwig]

On Tue, Sep 08, 2020 at 09:29:35AM +0200, Christoph Hellwig wrote:
> FYI, this is what I'd do relative to the patch on the dma-ranges
> branch.  In fact realizing this makes me want to refactor things a bit
> so that the new code can entirely live in the dma-direct code, but please
> test this first:

And of course this isn't going to work for arm devices without any
range, so let's try this instead:

diff --git a/arch/arm/include/asm/dma-mapping.h 
b/arch/arm/include/asm/dma-mapping.h
index c21893f683b585..e913e04d2be8b9 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -35,21 +35,20 @@ static inline const struct dma_map_ops 
*get_arch_dma_ops(struct bus_type *bus)
 #ifndef __arch_pfn_to_dma
 static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn)
 {
-   if (dev) {
-   phys_addr_t paddr = PFN_PHYS(pfn);
-
-   pfn -= PFN_DOWN(dma_offset_from_phys_addr(dev, paddr));
-   }
-   return (dma_addr_t)__pfn_to_bus(pfn);
+   if (!dev)
+   return (dma_addr_t)__pfn_to_bus(pfn);
+   if (dev->dma_range_map)
+   return translate_phys_to_dma(dev, PFN_PHYS(pfn));
+   return (dma_addr_t)PFN_PHYS(pfn);
 }
 
 static inline unsigned long dma_to_pfn(struct device *dev, dma_addr_t addr)
 {
-   unsigned long pfn = __bus_to_pfn(addr);
-
-   if (dev)
-   pfn += PFN_DOWN(dma_offset_from_dma_addr(dev, addr));
-   return pfn;
+   if (!dev)
+   return __bus_to_pfn(addr);
+   if (dev->dma_range_map)
+   return PFN_DOWN(translate_dma_to_phys(dev, addr));
+   return PFN_DOWN(addr);
 }
 
 static inline void *dma_to_virt(struct device *dev, dma_addr_t addr)
diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h
index 7831ca5b1b5dd6..e624171c4962ad 100644
--- a/include/linux/dma-direct.h
+++ b/include/linux/dma-direct.h
@@ -19,12 +19,16 @@ extern unsigned int zone_dma_bits;
 #else
 static inline dma_addr_t __phys_to_dma(struct device *dev, phys_addr_t paddr)
 {
-   return (dma_addr_t)paddr - dma_offset_from_phys_addr(dev, paddr);
+   if (dev->dma_range_map)
+   return (dma_addr_t)paddr - translate_phys_to_dma(dev, paddr);
+   return (dma_addr_t)paddr;
 }
 
-static inline phys_addr_t __dma_to_phys(struct device *dev, dma_addr_t 
dev_addr)
+static inline phys_addr_t __dma_to_phys(struct device *dev, dma_addr_t 
dma_addr)
 {
-   return (phys_addr_t)dev_addr + dma_offset_from_dma_addr(dev, dev_addr);
+   if (dev->dma_range_map)
+   return translate_dma_to_phys(dev, dma_addr);
+   return (phys_addr_t)dma_addr;
 }
 #endif /* !CONFIG_ARCH_HAS_PHYS_TO_DMA */
 
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 4c4646761afee4..3b1ceebb6f2ad5 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -199,29 +199,28 @@ struct bus_dma_region {
 };
 
 #ifdef CONFIG_HAS_DMA
-static inline u64 dma_offset_from_dma_addr(struct device *dev,
-   dma_addr_t dma_addr)
+static inline dma_addr_t translate_phys_to_dma(struct device *dev,
+   phys_addr_t paddr)
 {
-   const struct bus_dma_region *m = dev->dma_range_map;
+   const struct bus_dma_region *m;
 
-   if (m)
-   for (; m->size; m++)
-   if (dma_addr >= m->dma_start &&
-   dma_addr - m->dma_start < m->size)
-   return m->offset;
-   return 0;
+   for (m = dev->dma_range_map; m->size; m++)
+   if (paddr >= m->cpu_start && paddr - m->cpu_start < m->size)
+   return (dma_addr_t)paddr - m->offset;
+
+   /* make sure dma_capable fails when no translation is available */
+   return DMA_MAPPING_ERROR; 
 }
 
-static inline u64 dma_offset_from_phys_addr(struct device *dev,
-   phys_addr_t paddr)
+static inline phys_addr_t translate_dma_to_phys(struct device *dev,
+   dma_addr_t dma_addr)
 {
-   const struct bus_dma_region *m = dev->dma_range_map;
+   const struct bus_dma_region *m;
+
+   for (m = dev->dma_range_map; m->size; m++)
+   if (dma_addr >= m->dma_start && dma_addr - m->dma_start < 
m->size)
+   return (phys_addr_t)dma_addr + m->offset;
 
-   if (m)
-   for (; m->size; m++)
-   if (paddr >= m->cpu_start &&
-   paddr - m->cpu_start < m->size)
-   return m->offset;
return 0;
 }
 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v11 07/11] device-mapping: Introduce DMA range map, supplanting dma_pfn_offset

[Christoph Hellwig]

And because I like replying to myself so much, here is a link to the
version with the arm cleanup patch applied.  Unlike the previous two
attempts this has at least survived very basic sanity testing:

http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/dma-ranges.2

Note that we'll still need to sort out the arm/keystone warnings from
the original patch.  Do we have anyone on the CC list who knows that
platform a little better to figure out if the ifdef solution would work?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v11 00/11] PCI: brcmstb: enable PCIe for STB chips

[Christoph Hellwig]

On Tue, Sep 08, 2020 at 11:42:26AM +0100, Lorenzo Pieralisi wrote:
> > Maybe we can parallelize the PCIe driver review while the DMA changes
> > are being worked on in Christoph's branch. Lorenzo, are you fine with
> > the PCIe changes proper?
> 
> I will have a look - the main contentious point was about the DMA
> changes - if Christoph is happy with them I am OK with them
> too - I hope there is not anything controversial in the host
> bridge driver itself but I will look into it.

I'm pretty happy with the overall shape.  Now we just need to squeeze
out the regressions..
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v11 07/11] device-mapping: Introduce DMA range map, supplanting dma_pfn_offset

[Christoph Hellwig]

On Tue, Sep 08, 2020 at 01:20:56PM +0200, Nicolas Saenz Julienne wrote:
> On Tue, 2020-09-08 at 11:43 +0200, Christoph Hellwig wrote:
> > And because I like replying to myself so much, here is a link to the
> > version with the arm cleanup patch applied.  Unlike the previous two
> > attempts this has at least survived very basic sanity testing:
> > 
> > http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/dma-ranges.2
> > 
> > Note that we'll still need to sort out the arm/keystone warnings from
> > the original patch.  Do we have anyone on the CC list who knows that
> > platform a little better to figure out if the ifdef solution would work?
> 
> Had to do the following to boot without errors:

I've folded in.  That being said the whole RPi4 setup confuses the
heck out of me.  I wonder what thing was smoked to come up with it..
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFT/RFC 06/49] staging: media: zoran: unsplit lines

[Christoph Hellwig]

On Mon, Sep 21, 2020 at 10:19:41AM +, Corentin Labbe wrote:
> This patch un-split some lines.
> Signed-off-by: Corentin Labbe 

Just don't do this.  This is a purious change going over 80 chars for
absolutely no reason, and you'd still need a very good reason for that.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFT/RFC 24/49] staging: media: zoran: Use DMA coherent for stat_com

[Christoph Hellwig]

On Mon, Sep 21, 2020 at 10:19:59AM +, Corentin Labbe wrote:
> Instead of using a fragile virt_to_bus, let's use proper DMA coherent
> for the stat_com entry.
> 
> Signed-off-by: Corentin Labbe 
> ---
>  drivers/staging/media/zoran/zoran.h|  2 ++
>  drivers/staging/media/zoran/zoran_card.c   | 20 +---
>  drivers/staging/media/zoran/zoran_device.c |  3 +--
>  3 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/media/zoran/zoran.h 
> b/drivers/staging/media/zoran/zoran.h
> index aa2a8f688a01..8f3faa4eb60f 100644
> --- a/drivers/staging/media/zoran/zoran.h
> +++ b/drivers/staging/media/zoran/zoran.h
> @@ -351,6 +351,8 @@ struct zoran {
>   unsigned long frame_num;
>  
>   wait_queue_head_t test_q;
> +
> + dma_addr_t p_sc;
>  };
>  
>  static inline struct zoran *to_zoran(struct v4l2_device *v4l2_dev)
> diff --git a/drivers/staging/media/zoran/zoran_card.c 
> b/drivers/staging/media/zoran/zoran_card.c
> index 3a7895be3341..a8d23bf126c3 100644
> --- a/drivers/staging/media/zoran/zoran_card.c
> +++ b/drivers/staging/media/zoran/zoran_card.c
> @@ -931,11 +931,15 @@ static int zr36057_init(struct zoran *zr)
>   zoran_open_init_params(zr);
>  
>   /* allocate memory *before* doing anything to the hardware in case 
> allocation fails */
> - zr->stat_com = kzalloc(BUZ_NUM_STAT_COM * 4, GFP_KERNEL);
>   zr->video_dev = video_device_alloc();
> - if (!zr->stat_com || !zr->video_dev) {
> + if (!zr->video_dev) {
>   err = -ENOMEM;
> - goto exit_free;
> + goto exit;
> + }
> + zr->stat_com = dma_alloc_coherent(&zr->pci_dev->dev, BUZ_NUM_STAT_COM * 
> sizeof(u32), &zr->p_sc, GFP_KERNEL);

Again, don't write junk coe like this.  Stick to the 80 lines unless
you have a very good reason.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFT/RFC 01/49] staging: media: Revert "media: zoran: remove deprecated driver"

[Christoph Hellwig]

> + fh->buffers.buffer[i].v4l.fbuffer = mem;
> + fh->buffers.buffer[i].v4l.fbuffer_phys = virt_to_phys(mem);
> + fh->buffers.buffer[i].v4l.fbuffer_bus = virt_to_bus(mem);
> + for (off = 0; off < fh->buffers.buffer_size;
> +  off += PAGE_SIZE)
> + SetPageReserved(virt_to_page(mem + off));

This messing with SetPageReserved needs to go away before we bring
back the driver, even for staging.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: atomisp: stop compiling compat_ioctl32 code

[Christoph Hellwig]

On Wed, Oct 07, 2020 at 04:16:39PM +0200, Arnd Bergmann wrote:
> Alternatively, the entire file could just be removed, since anyone
> willing to restore the functionality can equally well just look up
> the contents in the git history if needed.

I suspect that is the right thing.  Note that given that the driver
is only in staging anyway, the right thing to do would be to change
the ioctl ABI to be more compat friendly to start with.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFC PKS/PMEM 24/58] fs/freevxfs: Utilize new kmap_thread()

[Christoph Hellwig]

> - kaddr = kmap(pp);
> + kaddr = kmap_thread(pp);
>   memcpy(kaddr, vip->vii_immed.vi_immed + offset, PAGE_SIZE);
> - kunmap(pp);
> + kunmap_thread(pp);

You only Cced me on this particular patch, which means I have absolutely
no idea what kmap_thread and kunmap_thread actually do, and thus can't
provide an informed review.

That being said I think your life would be a lot easier if you add
helpers for the above code sequence and its counterpart that copies
to a potential hughmem page first, as that hides the implementation
details from most users.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

use of dma_direct_set_offset in (allwinner) drivers

[Christoph Hellwig]

Hi all,

Linux 5.10-rc1 switched from having a single dma offset in struct device
to a set of DMA ranges, and introduced a new helper to set them,
dma_direct_set_offset.

This in fact surfaced that a bunch of drivers that violate our layering
and set the offset from drivers, which meant we had to reluctantly
export the symbol to set up the DMA range.

The drivers are:

drivers/gpu/drm/sun4i/sun4i_backend.c

  This just use dma_direct_set_offset as a fallback.  Is there any good
  reason to not just kill off the fallback?

drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c

  Same as above.

drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c

  This driver unconditionally sets the offset.  Why can't we do this
  in the device tree?

drivers/staging/media/sunxi/cedrus/cedrus_hw.c

  Same as above.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: use of dma_direct_set_offset in (allwinner) drivers

[Christoph Hellwig]

On Wed, Nov 04, 2020 at 10:15:49AM +, Robin Murphy wrote:
> How about having something in the platform code that keys off the top-level 
> SoC compatible and uses a bus notifier to create offsets for the relevant 
> devices if an MBUS description is missing? At least that way the workaround 
> could be confined to a single dedicated place and look somewhat similar to 
> other special cases like sta2x11, rather than being duplicated all over the 
> place.

Yes, that would be the right way to handle the issue.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/7] sunxi: Remove the calls to dma_direct_set_offset

[Christoph Hellwig]

Thanks,

this looks good to me:

Reviewed-by: Christoph Hellwig 

Can you include this patch at the end of your series to that it gets
picked up with the other patches?

---
>From 5963f88d365367fe74d477b8420d34562d684406 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig 
Date: Fri, 6 Nov 2020 17:02:17 +0100
Subject: dma-mapping: remove the dma_direct_set_offset export

Drop the dma_direct_set_offset export and move the declaration to
dma-map-ops.h now that the Allwinner drivers have stopped calling it.

Signed-off-by: Christoph Hellwig 
---
 arch/arm/mach-keystone/keystone.c | 2 +-
 arch/arm/mach-omap1/usb.c | 2 +-
 arch/sh/drivers/pci/pcie-sh7786.c | 2 +-
 arch/x86/pci/sta2x11-fixup.c  | 3 ++-
 include/linux/dma-map-ops.h   | 3 +++
 include/linux/dma-mapping.h   | 7 ---
 kernel/dma/direct.c   | 1 -
 7 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-keystone/keystone.c 
b/arch/arm/mach-keystone/keystone.c
index 09a65c2dfd7327..cd711bfc591f21 100644
--- a/arch/arm/mach-keystone/keystone.c
+++ b/arch/arm/mach-keystone/keystone.c
@@ -8,7 +8,7 @@
  */
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
diff --git a/arch/arm/mach-omap1/usb.c b/arch/arm/mach-omap1/usb.c
index ba8566204ea9f4..86d3b3c157af44 100644
--- a/arch/arm/mach-omap1/usb.c
+++ b/arch/arm/mach-omap1/usb.c
@@ -9,7 +9,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 
 #include 
diff --git a/arch/sh/drivers/pci/pcie-sh7786.c 
b/arch/sh/drivers/pci/pcie-sh7786.c
index 4468289ab2cac7..4d499476c33ad6 100644
--- a/arch/sh/drivers/pci/pcie-sh7786.c
+++ b/arch/sh/drivers/pci/pcie-sh7786.c
@@ -12,7 +12,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
diff --git a/arch/x86/pci/sta2x11-fixup.c b/arch/x86/pci/sta2x11-fixup.c
index 5701d5ba3df4ba..7d25256918543f 100644
--- a/arch/x86/pci/sta2x11-fixup.c
+++ b/arch/x86/pci/sta2x11-fixup.c
@@ -11,7 +11,8 @@
 #include 
 #include 
 #include 
-#include 
+#include 
+#include 
 #include 
 
 #define STA2X11_SWIOTLB_SIZE (4*1024*1024)
diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h
index a5f89fc4d6df16..03925e438ec3e5 100644
--- a/include/linux/dma-map-ops.h
+++ b/include/linux/dma-map-ops.h
@@ -226,6 +226,9 @@ struct page *dma_alloc_from_pool(struct device *dev, size_t 
size,
bool (*phys_addr_ok)(struct device *, phys_addr_t, size_t));
 bool dma_free_from_pool(struct device *dev, void *start, size_t size);
 
+int dma_direct_set_offset(struct device *dev, phys_addr_t cpu_start,
+   dma_addr_t dma_start, u64 size);
+
 #ifdef CONFIG_ARCH_HAS_DMA_COHERENCE_H
 #include 
 #elif defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 2aaed35b556df4..2e49996a8f391a 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -558,11 +558,4 @@ static inline int dma_mmap_wc(struct device *dev,
 #define dma_unmap_len_set(PTR, LEN_NAME, VAL)do { } while (0)
 #endif
 
-/*
- * Legacy interface to set up the dma offset map.  Drivers really should not
- * actually use it, but we have a few legacy cases left.
- */
-int dma_direct_set_offset(struct device *dev, phys_addr_t cpu_start,
-   dma_addr_t dma_start, u64 size);
-
 #endif /* _LINUX_DMA_MAPPING_H */
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 06c111544f61d6..002268262c9ad8 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -547,4 +547,3 @@ int dma_direct_set_offset(struct device *dev, phys_addr_t 
cpu_start,
dev->dma_range_map = map;
return 0;
 }
-EXPORT_SYMBOL_GPL(dma_direct_set_offset);
-- 
2.28.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/7] sunxi: Remove the calls to dma_direct_set_offset

[Christoph Hellwig]

On Mon, Nov 09, 2020 at 10:43:03AM +0100, Maxime Ripard wrote:
> Hi Christoph, Chen-Yu, Hans,
> 
> On Fri, Nov 06, 2020 at 05:07:37PM +0100, Christoph Hellwig wrote:
> > Thanks,
> > 
> > this looks good to me:
> > 
> > Reviewed-by: Christoph Hellwig 
> > 
> > Can you include this patch at the end of your series to that it gets
> > picked up with the other patches?
> 
> I guess the easiest to avoid bisection issues would be to merge all this
> through drm-misc, would that work for you?

Fine with me!
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/7] sunxi: Remove the calls to dma_direct_set_offset

[Christoph Hellwig]

On Mon, Nov 09, 2020 at 10:43:03AM +0100, Maxime Ripard wrote:
> Hi Christoph, Chen-Yu, Hans,
> 
> On Fri, Nov 06, 2020 at 05:07:37PM +0100, Christoph Hellwig wrote:
> > Thanks,
> > 
> > this looks good to me:
> > 
> > Reviewed-by: Christoph Hellwig 
> > 
> > Can you include this patch at the end of your series to that it gets
> > picked up with the other patches?
> 
> I guess the easiest to avoid bisection issues would be to merge all this
> through drm-misc, would that work for you?

Is this going to get picked up in drm-misc?  I don't see it in linux-next
so far.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 13/14] staging: android: ion: Do not sync CPU cache on map/unmap

[Christoph Hellwig]

On Wed, Jan 16, 2019 at 10:17:23AM -0600, Andrew F. Davis wrote:
> I wouldn't go as far as to remove them just yet.. Liam seems pretty
> adamant that they have valid uses. I'm just not sure performance is one
> of them, maybe in the case of software locks between devices or
> something where there needs to be a lot of back and forth interleaved
> access on small amounts of data?

We shouldn't add unused features to start with.  If mainline users for
them appear we can always add them back once we've reviewed the uses
cases and agree that they are sane and have no better solution.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 13/14] staging: android: ion: Do not sync CPU cache on map/unmap

[Christoph Hellwig]

On Fri, Jan 18, 2019 at 12:31:34PM -0800, Laura Abbott wrote:
> I thought about doing that, the problem is it becomes an ABI break for
> existing users which I really didn't want to do again. If it
> ends up being the last thing we do before moving out of staging,
> I'd consider doing it.

This is staging code, so any existing users by defintion do not matter.
Let's not drag Linux development down over this.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes

[Christoph Hellwig]

On Fri, Jan 18, 2019 at 10:37:46AM -0800, Liam Mark wrote:
> Add support for configuring dma mapping attributes when mapping
> and unmapping memory through dma_buf_map_attachment and
> dma_buf_unmap_attachment.
> 
> Signed-off-by: Liam Mark 

And who is going to decide which ones to pass?  And who documents
which ones are safe?

I'd much rather have explicit, well documented dma-buf flags that
might get translated to the DMA API flags, which are not error checked,
not very well documented and way to easy to get wrong.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes

[Christoph Hellwig]

On Sat, Jan 19, 2019 at 08:50:41AM -0800, Laura Abbott wrote:
> > And who is going to decide which ones to pass?  And who documents
> > which ones are safe?
> > 
> > I'd much rather have explicit, well documented dma-buf flags that
> > might get translated to the DMA API flags, which are not error checked,
> > not very well documented and way to easy to get wrong.
> > 
> 
> I'm not sure having flags in dma-buf really solves anything
> given drivers can use the attributes directly with dma_map
> anyway, which is what we're looking to do. The intention
> is for the driver creating the dma_buf attachment to have
> the knowledge of which flags to use.

Well, there are very few flags that you can simply use for all calls of
dma_map*.  And given how badly these flags are defined I just don't want
people to add more places where they indirectly use these flags, as
it will be more than enough work to clean up the current mess.

What flag(s) do you want to pass this way, btw?  Maybe that is where
the problem is.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] vmbus: Switch to use new generic UUID API

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes

[Christoph Hellwig]

On Mon, Jan 21, 2019 at 11:44:10AM -0800, Liam Mark wrote:
> The main use case is for allowing clients to pass in 
> DMA_ATTR_SKIP_CPU_SYNC in order to skip the default cache maintenance 
> which happens in dma_buf_map_attachment and dma_buf_unmap_attachment. In 
> ION the buffers aren't usually accessed from the CPU so this allows 
> clients to often avoid doing unnecessary cache maintenance.

This can't work.  The cpu can still easily speculate into this area.
Moreover in general these operations should be cheap if the addresses
aren't cached.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes

[Christoph Hellwig]

On Mon, Jan 21, 2019 at 12:20:42PM -0800, Liam Mark wrote:
> I have left this to clients, but if they own the buffer they can have the 
> knowledge as to whether CPU access is needed in that use case (example for 
> post-processing).

That is an API design which the user is more likely to get wrong than
right and thus does not pass the smell test.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [Linaro-mm-sig] [PATCH 2/4] staging: android: ion: Restrict cache maintenance to dma mapped memory

[Christoph Hellwig]

The CPU may only access DMA mapped memory if ownership has been
transferred back to the CPU using dma_sync_{single,sg}_to_cpu, and then
before the device can access it again ownership needs to be transferred
back to the device using dma_sync_{single,sg}_to_device.

> I've run some testing, and this patch does indeed fix the crash in
> dma_sync_sg_for_cpu when it tried to use the 0 dma_address from the sg
> list.
> 
> Tested-by: Ørjan Eide 
> 
> I tested this on an older kernel, v4.14, since the dma-mapping code
> moved, in v4.19, to ignore the dma_address and instead use sg_phys() to
> get a valid address from the page, which is always valid in the ion sg
> lists. While this wouldn't crash on newer kernels, it's still good to
> avoid the unnecessary work when no CMO is needed.

Can you also test is with CONFIG_DMA_API_DEBUG enabled, as that should
catch all the usual mistakes in DMA API usage, including the one found?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 0/7] binder: eliminate use of vmalloc space for binder buffers

[Christoph Hellwig]

On Fri, Feb 08, 2019 at 10:35:13AM -0800, Todd Kjos wrote:
> Binder buffers have always been mapped into kernel space
> via map_kernel_range_noflush() to allow the binder driver
> to modify the buffer before posting to userspace for
> processing.
> 
> In recent Android releases, the number of long-running
> binder processes has increased to the point that for
> 32-bit systems, there is a risk of running out of
> vmalloc space.
> 
> This patch set removes the persistent mapping of the
> binder buffers into kernel space. Instead, the binder
> driver creates temporary mappings with kmap() or
> kmap_atomic() to copy to or from the buffer only when
> necessary.

Is there any good reason to actually map the user memory to kernel
space instead of just using copy_{to,from}_user?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RESEND PATCH 0/7] Add FOLL_LONGTERM to GUP fast and use it

[Christoph Hellwig]

On Tue, Feb 19, 2019 at 09:30:33PM -0800, ira.we...@intel.com wrote:
> From: Ira Weiny 
> 
> Resending these as I had only 1 minor comment which I believe we have covered
> in this series.  I was anticipating these going through the mm tree as they
> depend on a cleanup patch there and the IB changes are very minor.  But they
> could just as well go through the IB tree.
> 
> NOTE: This series depends on my clean up patch to remove the write parameter
> from gup_fast_permitted()[1]
> 
> HFI1, qib, and mthca, use get_user_pages_fast() due to it performance
> advantages.  These pages can be held for a significant time.  But
> get_user_pages_fast() does not protect against mapping of FS DAX pages.

This I don't get - if you do lock down long term mappings performance
of the actual get_user_pages call shouldn't matter to start with.

What do I miss?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] erofs: move erofs out of staging

[Christoph Hellwig]

On Sun, Aug 18, 2019 at 11:11:54AM -0400, Theodore Y. Ts'o wrote:
> Note that of the mainstream file systems, ext4 and xfs don't guarantee
> that it's safe to blindly take maliciously provided file systems, such
> as those provided by a untrusted container, and mount it on a file
> system without problems.  As I recall, one of the XFS developers
> described file system fuzzing reports as a denial of service attack on
> the developers.

I think this greatly misrepresents the general attitute of the XFS
developers.  We take sanity checks for the modern v5 on disk format
very series, and put a lot of effort into handling corrupted file
systems as good as possible, although there are of course no guaranteeѕ.

The quote that you've taken out of context is for the legacy v4 format
that has no checksums and other integrity features.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] erofs: move erofs out of staging

[Christoph Hellwig]

On Sun, Aug 18, 2019 at 09:16:38AM -0700, Eric Biggers wrote:
> Ted's observation was about maliciously-crafted filesystems, though, so
> integrity-only features such as metadata checksums are irrelevant.  Also the
> filesystem version is irrelevant; anything accepted by the kernel code (even 
> if

I think allowing users to mount file systems (any of ours) without
privilege is a rather bad idea.  But that doesn't mean we should not be
as robust as we can.  Optionally disabling support for legacy formats
at compile and/or runtime is something we should actively look into as
well.

> it's legacy/deprecated) is open attack surface.
> 
> I personally consider it *mandatory* that we deal with this stuff.  But I can
> understand that we don't do a good job at it, so we shouldn't hold a new
> filesystem to an unfairly high standard relative to other filesystems...

I very much disagree.  We can't really force anyone to fix up old file
systems.  But we can very much hold new ones to (slightly) higher
standards.  Thats the only way to get the average quality up.  Some as
for things like code style - we can't magically fix up all old stuff,
but we can and usually do hold new code to higher standards.  (Often not
to standards as high as I'd personally prefer, btw).
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] erofs: move erofs out of staging

[Christoph Hellwig]

On Sun, Aug 18, 2019 at 10:29:38AM -0700, Eric Biggers wrote:
> Not sure what you're even disagreeing with, as I *do* expect new filesystems 
> to
> be held to a high standard, and to be written with the assumption that the
> on-disk data may be corrupted or malicious.  We just can't expect the bar to 
> be
> so high (e.g. no bugs) that it's never been attained by *any* filesystem even
> after years/decades of active development.  If the developers were careful, 
> the
> code generally looks robust, and they are willing to address such bugs as they
> are found, realistically that's as good as we can expect to get...

Well, the impression I got from Richards quick look and the reply to it is
that there is very little attempt to validate the ondisk data structure
and there is absolutely no priority to do so.  Which is very different
from there is a bug or two here and there.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Christoph Hellwig]

Can we please just review the damn thing and get it into the proper
tree?  That whole concept of staging file systems just has been one
fricking disaster, including Greg just moving not fully reviewed ones
over like erofs just because he feels like it.  I'm getting sick and
tired of this scheme.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Christoph Hellwig]

On Thu, Aug 29, 2019 at 08:39:55AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Aug 28, 2019 at 11:23:40PM -0700, Christoph Hellwig wrote:
> > Can we please just review the damn thing and get it into the proper
> > tree?  That whole concept of staging file systems just has been one
> > fricking disaster, including Greg just moving not fully reviewed ones
> > over like erofs just because he feels like it.  I'm getting sick and
> > tired of this scheme.
> 
> For this filesystem, it's going to be a _lot_ of work before that can
> happen, and I'd really like to have lots of people help out with it
> instead of it living in random github trees for long periods of time.

Did you actually look at the thing instead of blindly applying some
pile of crap?

It basically is a reimplementation of fs/fat/ not up to kernel standards
with a few indirections thrown in to also support exfat.  So no amount
of work on this codebase is really going to bring us forward.  Instead
someone how can spend a couple days on this and actually has file
systems to test it just needs to bring the low-level format bits over
to our well tested fs/fat codebase instead of duplicating it.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Christoph Hellwig]

On Thu, Aug 29, 2019 at 04:24:09PM +0800, Gao Xiang wrote:
> It seems I misunderstood your idea, sorry about that... EROFS
> properly uses vfs interfaces (e.g. we also considered RCU symlink
> lookup path at the very beginning of our design as Al said [1],
> except for mount interface as Al mentioned [2] (thanks him for
> taking some time on it), it was used for our debugging use),
> and it didn't cause any extra burden to vfs or other subsystems.

It would still have been a lot less effort for everyone without
the idiotic staging detour.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 01/24] erofs: add on-disk layout

[Christoph Hellwig]

> --- /dev/null
> +++ b/fs/erofs/erofs_fs.h
> @@ -0,0 +1,316 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR Apache-2.0 */
> +/*
> + * linux/fs/erofs/erofs_fs.h

Please remove the pointless file names in the comment headers.

> +struct erofs_super_block {
> +/*  0 */__le32 magic;   /* in the little endian */
> +/*  4 */__le32 checksum;/* crc32c(super_block) */
> +/*  8 */__le32 features;/* (aka. feature_compat) */
> +/* 12 */__u8 blkszbits; /* support block_size == PAGE_SIZE only */

Please remove all the byte offset comments.  That is something that can
easily be checked with gdb or pahole.

> +/* 64 */__u8 volume_name[16];   /* volume name */
> +/* 80 */__le32 requirements;/* (aka. feature_incompat) */
> +
> +/* 84 */__u8 reserved2[44];
> +} __packed; /* 128 bytes */

Please don't add __packed.  In this case I think you don't need it
(but double check with pahole), but even if you would need it using
proper padding fields and making sure all fields are naturally aligned
will give you much better code generation on architectures that don't
support native unaligned access.

> +/*
> + * erofs inode data mapping:
> + * 0 - inode plain without inline data A:
> + * inode, [xattrs], ... | ... | no-holed data
> + * 1 - inode VLE compression B (legacy):
> + * inode, [xattrs], extents ... | ...
> + * 2 - inode plain with inline data C:
> + * inode, [xattrs], last_inline_data, ... | ... | no-holed data
> + * 3 - inode compression D:
> + * inode, [xattrs], map_header, extents ... | ...
> + * 4~7 - reserved
> + */
> +enum {
> + EROFS_INODE_FLAT_PLAIN,

This one doesn't actually seem to be used.

> + EROFS_INODE_FLAT_COMPRESSION_LEGACY,

why are we adding a legacy field to a brand new file system?

> + EROFS_INODE_FLAT_INLINE,
> + EROFS_INODE_FLAT_COMPRESSION,
> + EROFS_INODE_LAYOUT_MAX

It seems like these come from the on-disk format, in which case they
should have explicit values assigned to them.

Btw, I think it generally helps file system implementation quality
if you use a separate header for the on-disk structures vs in-memory
structures, as that keeps it clear in everyones mind what needs to
stay persistent and what can be chenged easily.

> +static bool erofs_inode_is_data_compressed(unsigned int datamode)
> +{
> + if (datamode == EROFS_INODE_FLAT_COMPRESSION)
> + return true;
> + return datamode == EROFS_INODE_FLAT_COMPRESSION_LEGACY;
> +}

This looks like a really obsfucated way to write:

return datamode == EROFS_INODE_FLAT_COMPRESSION ||
datamode == EROFS_INODE_FLAT_COMPRESSION_LEGACY;

> +/* 28 */__le32 i_reserved2;
> +} __packed;

Sane comment as above.

> +
> +/* 32 bytes on-disk inode */
> +#define EROFS_INODE_LAYOUT_V1   0
> +/* 64 bytes on-disk inode */
> +#define EROFS_INODE_LAYOUT_V2   1
> +
> +struct erofs_inode_v2 {
> +/*  0 */__le16 i_advise;

Why do we have two inode version in a newly added file system?

> +#define ondisk_xattr_ibody_size(count)   ({\
> + u32 __count = le16_to_cpu(count); \
> + ((__count) == 0) ? 0 : \
> + sizeof(struct erofs_xattr_ibody_header) + \
> + sizeof(__u32) * ((__count) - 1); })

This would be much more readable as a function.

> +#define EROFS_XATTR_ENTRY_SIZE(entry) EROFS_XATTR_ALIGN( \
> + sizeof(struct erofs_xattr_entry) + \
> + (entry)->e_name_len + le16_to_cpu((entry)->e_value_size))

Same here.

> +/* available compression algorithm types */
> +enum {
> + Z_EROFS_COMPRESSION_LZ4,
> + Z_EROFS_COMPRESSION_MAX
> +};

Seems like an on-disk value again that should use explicitly assigned
numbers.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 03/24] erofs: add super block operations

[Christoph Hellwig]

On Fri, Aug 02, 2019 at 08:53:26PM +0800, Gao Xiang wrote:
> +static int __init erofs_init_inode_cache(void)
> +{
> + erofs_inode_cachep = kmem_cache_create("erofs_inode",
> +sizeof(struct erofs_vnode), 0,
> +SLAB_RECLAIM_ACCOUNT,
> +init_once);
> +
> + return erofs_inode_cachep ? 0 : -ENOMEM;

Please just use normal if/else.  Also having this function seems
entirely pointless.

> +static void erofs_exit_inode_cache(void)
> +{
> + kmem_cache_destroy(erofs_inode_cachep);
> +}

Same for this one.

> +static void free_inode(struct inode *inode)

Please use an erofs_ prefix for all your functions.

> +{
> + struct erofs_vnode *vi = EROFS_V(inode);

Why is this called vnode instead of inode?  That seems like a rather
odd naming for a Linux file system.

> +
> + /* be careful RCU symlink path (see ext4_inode_info->i_data)! */
> + if (is_inode_fast_symlink(inode))
> + kfree(inode->i_link);

is_inode_fast_symlink only shows up in a later patch.  And really
obsfucates the check here in the only caller as you can just do an
unconditional kfree here - i_link will be NULL except for the case
where you explicitly set it.

Also this code is nothing like ext4, so the code seems a little confusing.

> +static bool check_layout_compatibility(struct super_block *sb,
> +struct erofs_super_block *layout)
> +{
> + const unsigned int requirements = le32_to_cpu(layout->requirements);

Why is the variable name for the on-disk subperblock layout?  We usually
still calls this something with sb in the name, e.g. dsb. for disk
super block.

> + EROFS_SB(sb)->requirements = requirements;
> +
> + /* check if current kernel meets all mandatory requirements */
> + if (requirements & (~EROFS_ALL_REQUIREMENTS)) {
> + errln("unidentified requirements %x, please upgrade kernel 
> version",
> +   requirements & ~EROFS_ALL_REQUIREMENTS);
> + return false;
> + }
> + return true;

Note that normally we call this features, but that doesn't really
matter too much.

> +static int superblock_read(struct super_block *sb)
> +{
> + struct erofs_sb_info *sbi;
> + struct buffer_head *bh;
> + struct erofs_super_block *layout;
> + unsigned int blkszbits;
> + int ret;
> +
> + bh = sb_bread(sb, 0);

Is there any good reasons to use buffer heads like this in new code
vs directly using bios?

> +
> + sbi->blocks = le32_to_cpu(layout->blocks);
> + sbi->meta_blkaddr = le32_to_cpu(layout->meta_blkaddr);
> + sbi->islotbits = ffs(sizeof(struct erofs_inode_v1)) - 1;
> + sbi->root_nid = le16_to_cpu(layout->root_nid);
> + sbi->inos = le64_to_cpu(layout->inos);
> +
> + sbi->build_time = le64_to_cpu(layout->build_time);
> + sbi->build_time_nsec = le32_to_cpu(layout->build_time_nsec);
> +
> + memcpy(&sb->s_uuid, layout->uuid, sizeof(layout->uuid));
> + memcpy(sbi->volume_name, layout->volume_name,
> +sizeof(layout->volume_name));

s_uuid should preferably be a uuid_t (assuming it is a real BE uuid,
if it is le it should be a guid_t).

> +/* set up default EROFS parameters */
> +static void default_options(struct erofs_sb_info *sbi)
> +{
> +}

No need to add an empty function.

> +static int erofs_fill_super(struct super_block *sb, void *data, int silent)
> +{
> + struct inode *inode;
> + struct erofs_sb_info *sbi;
> + int err;
> +
> + infoln("fill_super, device -> %s", sb->s_id);
> + infoln("options -> %s", (char *)data);

That is some very verbose debug info.  We usually don't add that and
let people trace the function instead.  Also you should probably
implement the new mount API.
new mount API.

> +static void erofs_kill_sb(struct super_block *sb)
> +{
> + struct erofs_sb_info *sbi;
> +
> + WARN_ON(sb->s_magic != EROFS_SUPER_MAGIC);
> + infoln("unmounting for %s", sb->s_id);
> +
> + kill_block_super(sb);
> +
> + sbi = EROFS_SB(sb);
> + if (!sbi)
> + return;
> + kfree(sbi);
> + sb->s_fs_info = NULL;
> +}

Why is this needed?  You can just free your sb privatte information in
->put_super and wire up kill_block_super as the ->kill_sb method
directly.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 04/24] erofs: add raw address_space operations

[Christoph Hellwig]

The actual address_space operations seem to largely duplicate
the iomap versions.  Please use those instead.  Also I don't think
any new file system should write up ->bmap these days.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 05/24] erofs: add inode operations

[Christoph Hellwig]

On Fri, Aug 02, 2019 at 08:53:28PM +0800, Gao Xiang wrote:
> This adds core functions to get, read an inode.
> It adds statx support as well.
> 
> Signed-off-by: Gao Xiang 
> ---
>  fs/erofs/inode.c | 291 +++
>  1 file changed, 291 insertions(+)
>  create mode 100644 fs/erofs/inode.c
> 
> diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
> new file mode 100644
> index ..b6ea997bc4ae
> --- /dev/null
> +++ b/fs/erofs/inode.c
> @@ -0,0 +1,291 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * linux/fs/erofs/inode.c
> + *
> + * Copyright (C) 2017-2018 HUAWEI, Inc.
> + * http://www.huawei.com/
> + * Created by Gao Xiang 
> + */
> +#include "internal.h"
> +
> +#include 
> +
> +/* no locking */
> +static int read_inode(struct inode *inode, void *data)
> +{
> + struct erofs_vnode *vi = EROFS_V(inode);
> + struct erofs_inode_v1 *v1 = data;
> + const unsigned int advise = le16_to_cpu(v1->i_advise);
> + erofs_blk_t nblks = 0;
> +
> + vi->datamode = __inode_data_mapping(advise);

What is the deal with these magic underscores here and various
other similar helpers?

> + /* fast symlink (following ext4) */

This actually originates in FFS.  But it is so common that the comment
seems a little pointless.

> + if (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) {
> + char *lnk = erofs_kmalloc(sbi, inode->i_size + 1, GFP_KERNEL);

Please just use plain kmalloc everywhere and let the normal kernel
error injection code take care of injeting any errors.

> + /* inline symlink data shouldn't across page boundary as well */

... should not cross ..

> + if (unlikely(m_pofs + inode->i_size > PAGE_SIZE)) {
> + DBG_BUGON(1);
> + kfree(lnk);
> + return -EIO;
> + }
> +
> + /* get in-page inline data */

s/get/copy/, but the comment seems rather pointless.

> + memcpy(lnk, data + m_pofs, inode->i_size);
> + lnk[inode->i_size] = '\0';
> +
> + inode->i_link = lnk;
> + set_inode_fast_symlink(inode);

Please just set the ops directly instead of obsfucating that in a single
caller, single line inline function.  And please set it instead of the
normal symlink iops in the same place where you also set those.:w

> + err = read_inode(inode, data + ofs);
> + if (!err) {

if (err)
goto out_unlock;

.. and save one level of indentation.

> + if (is_inode_layout_compression(inode)) {

The name of this helper is a little odd.  But I think just
opencoding it seems generally cleaner anyway.


> + err = -ENOTSUPP;
> + goto out_unlock;
> + }
> +
> + inode->i_mapping->a_ops = &erofs_raw_access_aops;
> +
> + /* fill last page if inline data is available */
> + err = fill_inline_data(inode, data, ofs);

Well, I think you should move the is_inode_flat_inline and
(S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) checks from that
helper here, as otherwise you make everyone wonder why you'd always
fill out the inline data.

> +static inline struct inode *erofs_iget_locked(struct super_block *sb,
> +   erofs_nid_t nid)
> +{
> + const unsigned long hashval = erofs_inode_hash(nid);
> +
> +#if BITS_PER_LONG >= 64
> + /* it is safe to use iget_locked for >= 64-bit platform */
> + return iget_locked(sb, hashval);
> +#else
> + return iget5_locked(sb, hashval, erofs_ilookup_test_actor,
> + erofs_iget_set_actor, &nid);
> +#endif

Just use the slightly more complicated 32-bit version everywhere so that
you have a single actually tested code path.  And then remove this
helper.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 06/24] erofs: support special inode

[Christoph Hellwig]

On Fri, Aug 02, 2019 at 08:53:29PM +0800, Gao Xiang wrote:
> This patch adds to support special inode, such as
> block dev, char, socket, pipe inode.
> 
> Signed-off-by: Gao Xiang 
> ---
>  fs/erofs/inode.c | 27 +--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
> index b6ea997bc4ae..637bf6e4de44 100644
> --- a/fs/erofs/inode.c
> +++ b/fs/erofs/inode.c
> @@ -34,7 +34,16 @@ static int read_inode(struct inode *inode, void *data)
>   vi->xattr_isize = ondisk_xattr_ibody_size(v2->i_xattr_icount);
>  
>   inode->i_mode = le16_to_cpu(v2->i_mode);
> - vi->raw_blkaddr = le32_to_cpu(v2->i_u.raw_blkaddr);
> + if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
> + S_ISLNK(inode->i_mode))
> + vi->raw_blkaddr = le32_to_cpu(v2->i_u.raw_blkaddr);
> + else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode))
> + inode->i_rdev =
> + new_decode_dev(le32_to_cpu(v2->i_u.rdev));
> + else if (S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode))
> + inode->i_rdev = 0;
> + else
> + return -EIO;

Please use a switch statement when dealing with the file modes to
make everything easier to read.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 08/24] erofs: add namei functions

[Christoph Hellwig]

On Fri, Aug 02, 2019 at 08:53:31PM +0800, Gao Xiang wrote:
> +struct erofs_qstr {
> + const unsigned char *name;
> + const unsigned char *end;
> +};

Maybe erofs_name?  The q in qstr stands for quick, because of the
existing hash and len, which this doesn't really provide.

Also I don't really see why you don't just pass the actual qstr and
just document that dirnamecmp does not look at the hash and thus
doesn't require it to be filled out.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 01/24] erofs: add on-disk layout

[Christoph Hellwig]

On Thu, Aug 29, 2019 at 06:32:53PM +0800, Gao Xiang wrote:
> I can fix it up as you like but I still cannot get
> what is critical issues here.

The problem is that the whole codebase is way substandard quality,
looking a lot like Linux code from 20 years ago.  Yes, we already have
plenty of code of that standard in the tree, but we should not add more.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Christoph Hellwig]

On Thu, Aug 29, 2019 at 11:50:19AM +0200, Greg Kroah-Hartman wrote:
> I did try just that, a few years ago, and gave up on it.  I don't think
> it can be added to the existing vfat code base but I am willing to be
> proven wrong.

And what exactly was the problem?

> 
> Now that we have the specs, it might be easier, and the vfat spec is a
> subset of the exfat spec, but to get stuff working today, for users,
> it's good to have it in staging.  We can do the normal, "keep it in
> stable, get a clean-room implementation merged like usual, and then
> delete the staging version" three step process like we have done a
> number of times already as well.
> 
> I know the code is horrible, but I will gladly take horrible code into
> staging.  If it bothers you, just please ignore it.  That's what staging
> is there for :)

And then after a while you decide it's been long enough and force move
it out of staging like the POS erofs code?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Christoph Hellwig]

On Thu, Aug 29, 2019 at 01:18:10PM +0200, Greg Kroah-Hartman wrote:
> Hey, that's not nice, erofs isn't a POS.  It could always use more
> review, which the developers asked for numerous times.
> 
> There's nothing different from a filesystem compared to a driver.  If
> its stand-alone, and touches nothing else, all issues with it are
> self-contained and do not bother anyone else in the kernel.  We merge
> drivers all the time that need more work because our review cycles are
> low.  And review cycles for vfs developers are even more scarce than
> driver reviewers.

A lot of the issue that are trivial to pick are really just very basic
issue that don't even require file system know how.  Or in other ways
just a little less lazy developer that looks out for similar code
outside their own little fiefdom.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Christoph Hellwig]

On Thu, Aug 29, 2019 at 10:56:31PM +0200, Pali Rohár wrote:
> In my opinion, proper way should be to implement exFAT support into
> existing fs/fat/ code instead of replacing whole vfat/msdosfs by this
> new (now staging) fat implementation.
> 
> In linux kernel we really do not need two different implementation of
> VFAT32.

Not only not useful, but having another one is actively harmful, as
people might actually accidentally used it for classic fat.

But what I'm really annoyed at is this whole culture of just dumping
some crap into staging and hoping it'll sort itself out.  Which it
won't.  We'll need a dedidcated developer spending some time on it
and just get it into shape, and having it in staging does not help
with that at all - it will get various random cleanup that could
be trivially scripted, but that is rarely the main issue with any
codebase.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 2/7] erofs: some marcos are much more readable as a function

[Christoph Hellwig]

On Thu, Aug 29, 2019 at 08:16:27PM -0700, Joe Perches wrote:
> > -   sizeof(__u32) * ((__count) - 1); })
> > +static inline unsigned int erofs_xattr_ibody_size(__le16 d_icount)
> > +{
> > +   unsigned int icount = le16_to_cpu(d_icount);
> > +
> > +   if (!icount)
> > +   return 0;
> > +
> > +   return sizeof(struct erofs_xattr_ibody_header) +
> > +   sizeof(__u32) * (icount - 1);
> 
> Maybe use struct_size()?

Declaring a variable that is only used for struct_size is rather ugly.
But while we are nitpicking: you don't need to byteswap to check for 0,
so the local variable could be avoided.

Also what is that magic -1 for?  Normally we use that for the
deprecated style where a variable size array is declared using
varname[1], but that doesn't seem to be the case for erofs.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations

[Christoph Hellwig]

On Fri, Aug 30, 2019 at 11:36:42AM +0800, Gao Xiang wrote:
> As Dan Carpenter suggested [1], I have to remove
> all erofs likely/unlikely annotations.

Do you have to remove all of them, or just those where you don't have
a particularly good reason why you think in this particular case they
might actually matter?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 7/7] erofs: redundant assignment in __erofs_get_meta_page()

[Christoph Hellwig]

> - err = bio_add_page(bio, page, PAGE_SIZE, 0);
> - if (err != PAGE_SIZE) {
> + if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) {
>   err = -EFAULT;
>   goto err_out;
>   }

This patch looks like an improvement.  But looking at that whole
area just makes me cringe.

Why is there __erofs_get_meta_page with the two weird booleans instead
of a single erofs_get_meta_page that gets and gfp_t for additional
flags and an unsigned int for additional bio op flags.

Why do need ioprio support to start with?  Seeing that in a new
fs look kinda odd.  Do you have benchmarks that show the difference?

That function then calls erofs_grab_bio, which tries to handle a
bio_alloc failure, except that the function will not actually fail
due the mempool backing it.  It also seems like and awfully
huge function to inline.

Why is there __submit_bio which really just obsfucates what is
going on?  Also why is __submit_bio using bio_set_op_attrs instead
of opencode it as the comment right next to it asks you to?

Also I really don't understand why you can't just use read_cache_page
or even read_cache_page_gfp instead of __erofs_get_meta_page.
That function is a whole lot of duplication of functionality shared
by a lot of other file systems.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v8 20/24] erofs: introduce generic decompression backend

[Christoph Hellwig]

On Thu, Aug 15, 2019 at 12:41:51PM +0800, Gao Xiang wrote:
> +static bool use_vmap;
> +module_param(use_vmap, bool, 0444);
> +MODULE_PARM_DESC(use_vmap, "Use vmap() instead of vm_map_ram() (default 0)");

And how would anyone know which to pick? 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 03/24] erofs: add super block operations

[Christoph Hellwig]

On Thu, Aug 29, 2019 at 06:50:48PM +0800, Gao Xiang wrote:
> > Please use an erofs_ prefix for all your functions.
> 
> It is already a static function, I have no idea what is wrong here.

Which part of all wasn't clear?  Have you looked at the prefixes for
most functions in the various other big filesystems?

> > > + /* be careful RCU symlink path (see ext4_inode_info->i_data)! */
> > > + if (is_inode_fast_symlink(inode))
> > > + kfree(inode->i_link);
> > 
> > is_inode_fast_symlink only shows up in a later patch.  And really
> > obsfucates the check here in the only caller as you can just do an
> > unconditional kfree here - i_link will be NULL except for the case
> > where you explicitly set it.
> 
> I cannot fully understand your point (sorry about my English),
> I will reply you about this later.

With that I mean that you should:

 1) remove is_inode_fast_symlink and just opencode it in the few places
using it
 2) remove the check in this place entirely as it is not needed
 3) remove the comment quoted above as it is more confusing than not
having the comment

> > Is there any good reasons to use buffer heads like this in new code
> > vs directly using bios?
> 
> This page can save in bdev page cache, it contains not only the erofs
> superblock so it can be fetched in page cache later.

If you want it in the page cache why not use read_mapping_page or similar?

> > > +/* set up default EROFS parameters */
> > > +static void default_options(struct erofs_sb_info *sbi)
> > > +{
> > > +}
> > 
> > No need to add an empty function.
> 
> Later patch will fill this function.

Please only add the function in the patch actually adding the
functionality.

> > > +}
> > 
> > Why is this needed?  You can just free your sb privatte information in
> > ->put_super and wire up kill_block_super as the ->kill_sb method
> > directly.
> 
> See Al's comments,
> https://lore.kernel.org/r/20190720224955.gd17...@zeniv.linux.org.uk/

With that code it makes sense.  In this paticular patch it does not.
So please add it only when actually needed.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 04/24] erofs: add raw address_space operations

[Christoph Hellwig]

On Thu, Aug 29, 2019 at 07:46:11PM +0800, Gao Xiang wrote:
> Hi Christoph,
> 
> On Thu, Aug 29, 2019 at 03:17:21AM -0700, Christoph Hellwig wrote:
> > The actual address_space operations seem to largely duplicate
> > the iomap versions.  Please use those instead.  Also I don't think
> > any new file system should write up ->bmap these days.
> 
> iomap doesn't support tail-end packing inline data till now,
> I think Chao and I told you and Andreas before [1].
> 
> Since EROFS keeps a self-contained driver for now, we will use
> iomap if it supports tail-end packing inline data later.

Well, so work with the maintainers to enhance the core kernel.  That
is how Linux development works.  We've added various iomap enhancements
for gfs in the last merge windows, and we've added more for the brand
new zonefs file system we plan to merge for 5.4.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 05/24] erofs: add inode operations

[Christoph Hellwig]

On Thu, Aug 29, 2019 at 07:59:22PM +0800, Gao Xiang wrote:
> On Thu, Aug 29, 2019 at 03:24:26AM -0700, Christoph Hellwig wrote:
> 
> []
> 
> > 
> > > +
> > > + /* fill last page if inline data is available */
> > > + err = fill_inline_data(inode, data, ofs);
> > 
> > Well, I think you should move the is_inode_flat_inline and
> > (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) checks from that
> > helper here, as otherwise you make everyone wonder why you'd always
> > fill out the inline data.
> 
> Currently, fill_inline_data() only fills for fast symlink,
> later we can fill any tail-end block (such as dir block)
> for our requirements.

So change it when that later changes actually come in.  And even then
having the checks outside the function is a lot more obvious.

> And I think that is minor.

The problem is that each of these issues might appear minor on their
own.  But combined a lot of the coding style choices lead to code that
is more suitable an obsfucated code contest than the Linux kernel as
trying to understand even just a few places requires jumping through
tons of helpers with misleading names and spread over various files.

> The consideration is simply because iget_locked performs better
> than iget5_locked.

In what benchmark do the differences show up?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v8 20/24] erofs: introduce generic decompression backend

[Christoph Hellwig]

On Sat, Aug 31, 2019 at 12:52:17AM +0800, Gao Xiang wrote:
> Hi Christoph,
> 
> On Fri, Aug 30, 2019 at 09:35:34AM -0700, Christoph Hellwig wrote:
> > On Thu, Aug 15, 2019 at 12:41:51PM +0800, Gao Xiang wrote:
> > > +static bool use_vmap;
> > > +module_param(use_vmap, bool, 0444);
> > > +MODULE_PARM_DESC(use_vmap, "Use vmap() instead of vm_map_ram() (default 
> > > 0)");
> > 
> > And how would anyone know which to pick?
> 
> It has significant FIO benchmark difference on sequential read least on 
> arm64...
> I have no idea whether all platform vm_map_ram() behaves better than vmap(),
> so I leave an option for users here...

vm_map_ram is supposed to generally behave better.  So if it doesn't
please report that that to the arch maintainer and linux-mm so that
they can look into the issue.  Having user make choices of deep down
kernel internals is just a horrible interface.

Please talk to maintainers of other bits of the kernel if you see issues
and / or need enhancements.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 01/21] erofs: remove all the byte offset comments

[Christoph Hellwig]

On Sun, Sep 01, 2019 at 01:51:10PM +0800, Gao Xiang wrote:
> From: Gao Xiang 
> 
> As Christoph suggested [1], "Please remove all the byte offset comments.
> that is something that can easily be checked with gdb or pahole."

Looks good.  If you want to keep them after the field names as someone
pointed out feel free to - I don't think it actually is very useful
but we've also heard other opinions.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 02/21] erofs: on-disk format should have explicitly assigned numbers

[Christoph Hellwig]

On Sun, Sep 01, 2019 at 01:51:11PM +0800, Gao Xiang wrote:
>  enum {
> - EROFS_INODE_FLAT_PLAIN,
> - EROFS_INODE_FLAT_COMPRESSION_LEGACY,
> - EROFS_INODE_FLAT_INLINE,
> - EROFS_INODE_FLAT_COMPRESSION,
> + EROFS_INODE_FLAT_PLAIN  = 0,
> + EROFS_INODE_FLAT_COMPRESSION_LEGACY = 1,
> + EROFS_INODE_FLAT_INLINE = 2,
> + EROFS_INODE_FLAT_COMPRESSION= 3,
>   EROFS_INODE_LAYOUT_MAX
>  };
>  
> @@ -184,7 +184,7 @@ struct erofs_xattr_entry {
>  
>  /* available compression algorithm types */
>  enum {
> - Z_EROFS_COMPRESSION_LZ4,
> + Z_EROFS_COMPRESSION_LZ4 = 0,
>   Z_EROFS_COMPRESSION_MAX
>  };

This all looks ok - it somtimes also helps to have a comment near
the numbers to indicate where they are stored, must that isn't a must.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 03/21] erofs: some macros are much more readable as a function

[Christoph Hellwig]

This looks much better now.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 04/21] erofs: kill __packed for on-disk structures

[Christoph Hellwig]

On Sun, Sep 01, 2019 at 01:51:13PM +0800, Gao Xiang wrote:
> From: Gao Xiang 
> 
> As Christoph suggested "Please don't add __packed" [1],
> remove all __packed except struct erofs_dirent here.
> 
> Note that all on-disk fields except struct erofs_dirent
> (12 bytes with a 8-byte nid) in EROFS are naturally aligned.

Thanks.  The users of various architectures where this generates a lot
better code will thank you.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 05/21] erofs: update erofs_inode_is_data_compressed helper

[Christoph Hellwig]

On Sun, Sep 01, 2019 at 01:51:14PM +0800, Gao Xiang wrote:
> From: Gao Xiang 
> 
> As Christoph said, "This looks like a really obsfucated
> way to write:
>   return datamode == EROFS_INODE_FLAT_COMPRESSION ||
>   datamode == EROFS_INODE_FLAT_COMPRESSION_LEGACY; "
> 
> Although I had my own consideration, it's the right way for now.

Well, if you do check one field for two values it really helps to do
the same style of check for both.  All your choice how you do the check,
but don't mix multiple styles..

So this looks good.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 06/21] erofs: kill erofs_{init,exit}_inode_cache

[Christoph Hellwig]

On Sun, Sep 01, 2019 at 01:51:15PM +0800, Gao Xiang wrote:
> From: Gao Xiang 
> 
> As Christoph said [1] "having this function seems
> entirely pointless", let's kill those.

Looks much better.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 07/21] erofs: use erofs_inode naming

[Christoph Hellwig]

>  {
> - struct erofs_vnode *vi = ptr;
> -
> - inode_init_once(&vi->vfs_inode);
> + inode_init_once(&((struct erofs_inode *)ptr)->vfs_inode);

Why doesn't this use EROFS_I?  This looks a little odd.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 09/21] erofs: update erofs symlink stuffs

[Christoph Hellwig]

Thanks, this looks much better.

>  fs/erofs/inode.c| 35 ++-
>  fs/erofs/internal.h | 10 --
>  fs/erofs/super.c|  5 ++---
>  3 files changed, 12 insertions(+), 38 deletions(-)

And that diffstat ain't bad either.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 10/21] erofs: kill is_inode_layout_compression()

[Christoph Hellwig]

Thanks,

this looks much better.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 11/21] erofs: use dsb instead of layout for ondisk super_block

[Christoph Hellwig]

> + dsb = (struct erofs_super_block *)((u8 *)bh->b_data +
> +EROFS_SUPER_OFFSET);

Not new in this patch, but that u8 cast shouldn't be needed.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 12/21] erofs: kill verbose debug info in erofs_fill_super

[Christoph Hellwig]

On Sun, Sep 01, 2019 at 01:51:21PM +0800, Gao Xiang wrote:
> From: Gao Xiang 
> 
> As Christoph said [1], "That is some very verbose
> debug info.  We usually don't add that and let
> people trace the function instead. "

Note that this applies to most of the infoln users as far as
I can tell.  And if you want to keep some of those I think you
should converted them to use pr_info directly, and also print
sb->s_id as a prefix before the actual message so that the user
knows which file system is affected.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 13/21] erofs: simplify erofs_grab_bio() since bio_alloc() never fail

[Christoph Hellwig]

On Sun, Sep 01, 2019 at 01:51:22PM +0800, Gao Xiang wrote:
> From: Gao Xiang 
> 
> As Christoph pointed out [1], "erofs_grab_bio tries to
> handle a bio_alloc failure, except that the function will
> not actually fail due the mempool backing it."
> 
> Sorry about useless code, fix it now.

A lot of file systems used to have this, it is a classic that gets
copy and pasted everywhere.  If you feel like doing a little project
it might make sense to check for other places that do the same.

> + bio = erofs_grab_bio(sb, blkaddr, 1, sb, read_endio);
>  
>   if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) {
>   err = -EFAULT;
> @@ -275,13 +270,7 @@ static inline struct bio *erofs_read_raw_page(struct bio 
> *bio,
>   if (nblocks > BIO_MAX_PAGES)
>   nblocks = BIO_MAX_PAGES;
>  
> - bio = erofs_grab_bio(sb, blknr, nblocks, sb,
> -  read_endio, false);
> - if (IS_ERR(bio)) {
> - err = PTR_ERR(bio);
> - bio = NULL;
> - goto err_out;
> - }
> + bio = erofs_grab_bio(sb, blknr, nblocks, sb, read_endio);

Btw, I think you should remove erofs_grab_bio in its current form.
The full code in data.c is indeed used in two places, so a local
erofs_raw_page_alloc_bio (or whatever name you prefer) helper here
that hardcodes the sb amd read_endio argument to simplify it a bit
makes sense.

> diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
> index f06a2fad7af2..abe28565d6c0 100644
> --- a/fs/erofs/zdata.c
> +++ b/fs/erofs/zdata.c
> @@ -1265,7 +1265,7 @@ static bool z_erofs_vle_submit_all(struct super_block 
> *sb,
>   if (!bio) {
>   bio = erofs_grab_bio(sb, first_index + i,
>BIO_MAX_PAGES, bi_private,
> -  z_erofs_vle_read_endio, true);
> +  z_erofs_vle_read_endio);

But I think here you might as well open code it entirely, or at least
us a separate (and local to zdata.c) helper.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 14/21] erofs: kill prio and nofail of erofs_get_meta_page()

[Christoph Hellwig]

Thanks,

much better.  I'm still hoping REQ_PRIO in this current form will go
entirely away soon as well.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 16/21] erofs: kill magic underscores

[Christoph Hellwig]

>  
> - vi->datamode = __inode_data_mapping(advise);
> + vi->datamode = erofs_inode_data_mapping(advise);
>  
>   if (vi->datamode >= EROFS_INODE_LAYOUT_MAX) {

While you are at it can we aim for some naming consistency here?  The
inode member is called is called datamode, the helper is called
inode_data_mapping, and the enum uses layout?  To me data_layout seems
most descriptive, datamode is probably ok, but mapping seems very
misleading given that we've already overloaded that terms for multiple
other uses.

And while we are at naming choices - maybe i_format might be
a better name for the i_advise field in the on-disk inode?

> + if (erofs_inode_version(advise) == EROFS_INODE_LAYOUT_V2) {

I still need to wade through the old thread - but didn't you say this
wasn't really a new vs old version but a compat vs full inode?  Maybe
the names aren't that suitable either?

>   struct erofs_inode_v2 *v2 = data;
>  
>   vi->inode_isize = sizeof(struct erofs_inode_v2);
> @@ -58,7 +58,7 @@ static int read_inode(struct inode *inode, void *data)
>   /* total blocks for compressed files */
>   if (erofs_inode_is_data_compressed(vi->datamode))
>   nblks = le32_to_cpu(v2->i_u.compressed_blocks);
> - } else if (__inode_version(advise) == EROFS_INODE_LAYOUT_V1) {
> + } else if (erofs_inode_version(advise) == EROFS_INODE_LAYOUT_V1) {

Also a lot of code would use a switch statements to switch for different
matches on the same value instead of chained if/else if/else if
statements.

> +#define erofs_bitrange(x, bit, bits) (((x) >> (bit)) & ((1 << (bits)) - 1))

> +#define erofs_inode_version(advise)  \
> + erofs_bitrange(advise, EROFS_I_VERSION_BIT, EROFS_I_VERSION_BITS)
>  
> +#define erofs_inode_data_mapping(advise) \
> + erofs_bitrange(advise, EROFS_I_DATA_MAPPING_BIT, \
> +EROFS_I_DATA_MAPPING_BITS)

All these should probably be inline functions.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 17/21] erofs: use a switch statement when dealing with the file modes

[Christoph Hellwig]

Thanks,

this looks much nicer.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 18/21] erofs: add "erofs_" prefix for common and short functions

[Christoph Hellwig]

Thanks.  I don't have a tree with all these applies, but please make
sure this covers at least all inlines in headers and all methods
wired up to operation structures.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 19/21] erofs: kill all erofs specific fault injection

[Christoph Hellwig]

On Sun, Sep 01, 2019 at 01:51:28PM +0800, Gao Xiang wrote:
> From: Gao Xiang 
> 
> As Christoph suggested [1], "Please just use plain kmalloc
> everywhere and let the normal kernel error injection code
> take care of injeting any errors."

Thanks.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 21/21] erofs: save one level of indentation

[Christoph Hellwig]

On Sun, Sep 01, 2019 at 01:51:30PM +0800, Gao Xiang wrote:
> From: Gao Xiang 
> 
> As Christoph said [1], ".. and save one
> level of indentation."

Thanks.  Just a little cleanup, but cumulated things like this really
help readability.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 20/21] erofs: kill use_vmap module parameter

[Christoph Hellwig]

> @@ -224,9 +220,6 @@ static void *erofs_vmap(struct page **pages, unsigned int 
> count)
>  {
>   int i = 0;
>  
> - if (use_vmap)
> - return vmap(pages, count, VM_MAP, PAGE_KERNEL);
> -
>   while (1) {
>   void *addr = vm_map_ram(pages, count, -1, PAGE_KERNEL);

I think you can just open code this in the caller.

>  static void erofs_vunmap(const void *mem, unsigned int count)
>  {
> - if (!use_vmap)
> - vm_unmap_ram(mem, count);
> - else
> - vunmap(mem);
> + vm_unmap_ram(mem, count);
>  }

And this wrapper can go away entirely.

And don't forget to report your performance observations to the arm64
maintainers!
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 01/24] erofs: add on-disk layout

[Christoph Hellwig]

On Sun, Sep 01, 2019 at 03:54:11PM +0800, Gao Xiang wrote:
> It could be better has a name though, because 1) erofs.mkfs uses this
> definition explicitly, and we keep this on-disk definition erofs_fs.h
> file up with erofs-utils.
> 
> 2) For kernel use, first we have,
>datamode < EROFS_INODE_LAYOUT_MAX; and
>!erofs_inode_is_data_compressed, so there are only two mode here,
> 1) EROFS_INODE_FLAT_INLINE,
> 2) EROFS_INODE_FLAT_PLAIN
>if its datamode isn't EROFS_INODE_FLAT_INLINE (tail-end block packing),
>it should be EROFS_INODE_FLAT_PLAIN.
> 
>The detailed logic in erofs_read_inode and
>erofs_map_blocks_flatmode

Ok.  At least the explicit numbering makes this a little more obvious
now.  What seems fairly odd is that there are only various places that
check for some inode layouts/formats but nothing that does a switch
over all of them.

> > why are we adding a legacy field to a brand new file system?
> 
> The difference is just EROFS_INODE_FLAT_COMPRESSION_LEGACY doesn't
> have z_erofs_map_header, so it only supports default (4k clustersize)
> fixed-sized output compression rather than per-file setting, nothing
> special at all...

It still seems odd to add a legacy field to a brand new file system.

> > structures, as that keeps it clear in everyones mind what needs to
> > stay persistent and what can be chenged easily.
> 
> All fields in this file are on-disk representation by design
> (no logic for in-memory presentation).

Ok, make sense.Maybe add a note to the top of the file comment
that this is the on-disk format.

One little oddity is that erofs_inode_is_data_compressed is here, while
is_inode_flat_inline is in internal.h.  There are arguments for either
place, but I'd suggest to keep the related macros together.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 00/21] erofs: patchset addressing Christoph's comments

[Christoph Hellwig]

On Sun, Sep 01, 2019 at 01:51:09PM +0800, Gao Xiang wrote:
> Hi,
> 
> This patchset is based on the following patch by Pratik Shinde,
> https://lore.kernel.org/linux-erofs/20190830095615.10995-1-pratikshinde...@gmail.com/
> 
> All patches addressing Christoph's comments on v6, which are trivial,
> most deleted code are from erofs specific fault injection, which was
> followed f2fs and previously discussed in earlier topic [1], but
> let's follow what Christoph's said now.

I like where the cleanups are going.  But this is really just basic
code quality stuff.  We're not addressing the issues with large amounts
of functionality duplicating VFS helpers.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 07/21] erofs: use erofs_inode naming

[Christoph Hellwig]

On Mon, Sep 02, 2019 at 08:13:06PM +0800, Gao Xiang wrote:
> Hi Christoph,
> 
> On Mon, Sep 02, 2019 at 05:10:21AM -0700, Christoph Hellwig wrote:
> > >  {
> > > - struct erofs_vnode *vi = ptr;
> > > -
> > > - inode_init_once(&vi->vfs_inode);
> > > + inode_init_once(&((struct erofs_inode *)ptr)->vfs_inode);
> > 
> > Why doesn't this use EROFS_I?  This looks a little odd.
> 
> Thanks for your reply and suggestion...
> EROFS_I seems the revert direction ---> inode to erofs_inode
> here we need "erofs_inode" to inode...
> 
> Am I missing something? Hope not

No, you are not.  But the cast still looks odd.  Why not:

struct erofs_inode *ei = ptr;

inode_init_once(&ei->vfs_inode);
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 03/24] erofs: add super block operations

[Christoph Hellwig]

On Sun, Sep 01, 2019 at 04:54:55PM +0800, Gao Xiang wrote:
> No modification at this... (some comments already right here...)

>  20 /* 128-byte erofs on-disk super block */
>  21 struct erofs_super_block {
> ...
>  24 __le32 features;/* (aka. feature_compat) */
> ...
>  38 __le32 requirements;/* (aka. feature_incompat) */
> ...
>  41 };

This is only cosmetic, why not stick to feature_compat and
feature_incompat?

> > > + bh = sb_bread(sb, 0);
> > 
> > Is there any good reasons to use buffer heads like this in new code
> > vs directly using bios?
> 
> As you said, I want it in the page cache.
> 
> The reason "why not use read_mapping_page or similar?" is simply
> read_mapping_page -> .readpage -> (for bdev inode) block_read_full_page
>  -> create_page_buffers anyway...
> 
> sb_bread haven't obsoleted... It has similar function though...

With the different that it keeps you isolated from the buffer_head
internals.  This seems to be your only direct use of buffer heads,
which while not deprecated are a bit of an ugly step child.  So if
you can easily avoid creating a buffer_head dependency in a new
filesystem I think you should avoid it.

> > > + sbi->build_time = le64_to_cpu(layout->build_time);
> > > + sbi->build_time_nsec = le32_to_cpu(layout->build_time_nsec);
> > > +
> > > + memcpy(&sb->s_uuid, layout->uuid, sizeof(layout->uuid));
> > > + memcpy(sbi->volume_name, layout->volume_name,
> > > +sizeof(layout->volume_name));
> > 
> > s_uuid should preferably be a uuid_t (assuming it is a real BE uuid,
> > if it is le it should be a guid_t).
> 
> For this case, I have no idea how to deal with...
> I have little knowledge about this uuid stuff, so I just copied
> from f2fs... (Could be no urgent of this field...)

Who fills out this field in the on-disk format and how?

> The background is Al's comments in erofs v2
> (which simplify erofs_fill_super logic)
> https://lore.kernel.org/r/20190720224955.gd17...@zeniv.linux.org.uk/
> 
> with a specific notation...
> https://lore.kernel.org/r/20190721040547.gf17...@zeniv.linux.org.uk/
> 
> "
> > OTOH, for the case of NULL ->s_root ->put_super() won't be called
> > at all, so in that case you need it directly in ->kill_sb().
> "

Yes.  Although none of that is relevant for this initial version,
just after more features are added.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 05/24] erofs: add inode operations

[Christoph Hellwig]

On Sun, Sep 01, 2019 at 05:34:00PM +0800, Gao Xiang wrote:
> > > + return iget5_locked(sb, hashval, erofs_ilookup_test_actor,
> > > + erofs_iget_set_actor, &nid);
> > > +#endif
> > 
> > Just use the slightly more complicated 32-bit version everywhere so that
> > you have a single actually tested code path.  And then remove this
> > helper.
> 
> As I said before, 64-bit platforms is common currently,
> I think iget_locked is enough.
> https://lore.kernel.org/r/20190830184606.GA175612@architecture4/

The problem with that is that you now have two entirely different
code paths.  And the 32-bit one will probably get very little testing
and eventually bitrot.  We defintively had problems of that sort in
XFS in the past, so my suggestion is to not go down the root of
separate code for 32-bit vs 64-bit unless it makes a real difference
for a real-life workload.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 16/21] erofs: kill magic underscores

[Christoph Hellwig]

On Mon, Sep 02, 2019 at 08:39:37PM +0800, Gao Xiang wrote:
> > 
> > > + if (erofs_inode_version(advise) == EROFS_INODE_LAYOUT_V2) {
> > 
> > I still need to wade through the old thread - but didn't you say this
> > wasn't really a new vs old version but a compat vs full inode?  Maybe
> > the names aren't that suitable either?
> 
> Could you kindly give some suggestions for better naming about it?
> there are all supported by EROFS... (Actually we only mainly use v1...)

Maybe EROFS_INODE_LAYOUT_COMPACT and EROFS_INODE_LAYOUT_EXTENDED?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v8 11/24] erofs: introduce xattr & posixacl support

[Christoph Hellwig]

> +config EROFS_FS_XATTR
> + bool "EROFS extended attributes"
> + depends on EROFS_FS
> + default y
> + help
> +   Extended attributes are name:value pairs associated with inodes by
> +   the kernel or by users (see the attr(5) manual page, or visit
> +    for details).
> +
> +   If unsure, say N.
> +
> +config EROFS_FS_POSIX_ACL
> + bool "EROFS Access Control Lists"
> + depends on EROFS_FS_XATTR
> + select FS_POSIX_ACL
> + default y

Is there any good reason to make these optional these days?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v8 11/24] erofs: introduce xattr & posixacl support

[Christoph Hellwig]

On Mon, Sep 02, 2019 at 04:20:37PM +0200, David Sterba wrote:
> Oh right, I think the reasons are historical and that we can remove the
> options nowadays. From the compatibility POV this should be safe, with
> ACLs compiled out, no tool would use them, and no harm done when the
> code is present but not used.
> 
> There were some efforts by embedded guys to make parts of kernel more
> configurable to allow removing subsystems to reduce the final image
> size. In this case I don't think it would make any noticeable
> difference, eg. the size of fs/btrfs/acl.o on release config is 1.6KiB,
> while the whole module is over 1.3MiB.

Given that the erofs folks have an actual use case for it I think
it is fine to keep the options, I just wanted to ensure this wasn't
copy and pasted for no good reason.  Note that for XFS we don't have
an option for xattrs, as those are integral to a lot of file system
features.  WE have an ACL option mostly for historic reasons.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 03/24] erofs: add super block operations

[Christoph Hellwig]

On Mon, Sep 02, 2019 at 10:43:04PM +0800, Gao Xiang wrote:
> Hi Christoph,
> > > ...
> > >  24 __le32 features;/* (aka. feature_compat) */
> > > ...
> > >  38 __le32 requirements;/* (aka. feature_incompat) */
> > > ...
> > >  41 };
> > 
> > This is only cosmetic, why not stick to feature_compat and
> > feature_incompat?
> 
> Okay, will fix. (however, in my mind, I'm some confused why
> "features" could be incompatible...)

The feature is incompatible if it requires changes to the driver.
An easy to understand historic example is that ext3 originally did not
have the file types in the directory entry.  Adding them means old
file system drivers can not read a file system with this new feature,
so an incompat flag has to be added.

> > > > > + memcpy(&sb->s_uuid, layout->uuid, sizeof(layout->uuid));
> > > > > + memcpy(sbi->volume_name, layout->volume_name,
> > > > > +sizeof(layout->volume_name));
> > > > 
> > > > s_uuid should preferably be a uuid_t (assuming it is a real BE uuid,
> > > > if it is le it should be a guid_t).
> > > 
> > > For this case, I have no idea how to deal with...
> > > I have little knowledge about this uuid stuff, so I just copied
> > > from f2fs... (Could be no urgent of this field...)
> > 
> > Who fills out this field in the on-disk format and how?
> 
> mkfs.erofs, but this field leaves 0 for now. Is that reasonable?
> (using libuuid can generate it easily...)

If the filed is always zero for now please don't fill it out.  If you
decide it is worth adding the uuid eventually please add a compat
feature flag that you have a valid uuid and only fill out the field
if the file system actualy has a valid uuid.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 00/21] erofs: patchset addressing Christoph's comments

[Christoph Hellwig]

On Mon, Sep 02, 2019 at 10:24:52PM +0800, Gao Xiang wrote:
> > code quality stuff.  We're not addressing the issues with large amounts
> > of functionality duplicating VFS helpers.
> 
> You means killing erofs_get_meta_page or avoid erofs_read_raw_page?
> 
>  - For killing erofs_get_meta_page, here is the current erofs_get_meta_page:

> I think it is simple enough. read_cache_page need write a similar
> filler, or read_cache_page_gfp will call .readpage, and then
> introduce buffer_heads, that is what I'd like to avoid now (no need these
> bd_inode buffer_heads in memory...)

If using read_cache_page_gfp and ->readpage works, please do.  The
fact that the block device inode uses buffer heads is an implementation
detail that might not last very long and should be invisible to you.
It also means you can get rid of a lot of code that you don't have
to maintain and others don't have to update for global API changes.

>  - For erofs_read_raw_page, it can be avoided after iomap tail-end packing
>feature is done... If we remove it now, it will make EROFS broken.
>It is no urgent and Chao will focus on iomap tail-end packing feature.

Ok.  I wish we would have just sorted this out beforehand, which we
could have trivially done without all that staging mess.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 00/21] erofs: patchset addressing Christoph's comments

[Christoph Hellwig]

On Mon, Sep 02, 2019 at 11:50:38PM +0800, Gao Xiang wrote:
> > > You means killing erofs_get_meta_page or avoid erofs_read_raw_page?
> > > 
> > >  - For killing erofs_get_meta_page, here is the current 
> > > erofs_get_meta_page:
> > 
> > > I think it is simple enough. read_cache_page need write a similar
> > > filler, or read_cache_page_gfp will call .readpage, and then
> > > introduce buffer_heads, that is what I'd like to avoid now (no need these
> > > bd_inode buffer_heads in memory...)
> > 
> > If using read_cache_page_gfp and ->readpage works, please do.  The
> > fact that the block device inode uses buffer heads is an implementation
> > detail that might not last very long and should be invisible to you.
> > It also means you can get rid of a lot of code that you don't have
> > to maintain and others don't have to update for global API changes.
> 
> I care about those useless buffer_heads in memory for our products...
> 
> Since we are nobh filesystem (a little request, could I use it
> after buffer_heads are fully avoided, I have no idea why I need
> those buffer_heads in memory But I think bd_inode is good
> for caching metadata...)

Then please use read_cache_page with iomap_readpage(s), and write
comment explaining why your are not using read_cache_page_gfp.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 00/21] erofs: patchset addressing Christoph's comments

[Christoph Hellwig]

On Tue, Sep 03, 2019 at 04:17:49PM +0800, Gao Xiang wrote:
> I implement a prelimitary version, but I have no idea it is a really
> cleanup for now.

The fact that this has to guess the block device address_space
implementation is indeed pretty ugly.  I'd much prefer to just use
read_cache_page_gfp, and live with the fact that this allocates
bufferheads behind you for now.  I'll try to speed up my attempts to
get rid of the buffer heads on the block device mapping instead.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 00/25] erofs: patchset addressing Christoph's comments

[Christoph Hellwig]

On Wed, Sep 04, 2019 at 10:08:47AM +0800, Gao Xiang wrote:
> Hi,
> 
> This patchset is based on the following patch by Pratik Shinde,
> https://lore.kernel.org/linux-erofs/20190830095615.10995-1-pratikshinde...@gmail.com/
> 
> All patches addressing Christoph's comments on v6, which are trivial,
> most deleted code are from erofs specific fault injection, which was
> followed f2fs and previously discussed in earlier topic [1], but
> let's follow what Christoph's said now.
> 
> Comments and suggestions are welcome...

Do you have a git branch available somewhere containing the state
with all these patches applied?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 00/25] erofs: patchset addressing Christoph's comments

[Christoph Hellwig]

On Thu, Sep 05, 2019 at 09:03:54AM +0800, Gao Xiang wrote:
> Could we submit these patches if these patches look good...
> Since I'd like to work based on these patches, it makes a difference
> to the current code though...

Yes, I'm fine with these patches.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [GIT PULL] Staging/IIO driver patches for 5.4-rc1

[Christoph Hellwig]

Just as a note of record:  I don't think either file system move
is a good idea.  erofs still needs a lot of work, especially in
interacting with the mm code like abusing page->mapping.

exfat is just a random old code drop from Samsung hastily picked up,
and also duplicating the fat16/32 functionality, and without
consultation of the developes of that who are trying to work through
their process of contributing the uptodate and official version of it.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [GIT PULL] Staging/IIO driver patches for 5.4-rc1

[Christoph Hellwig]

On Wed, Sep 18, 2019 at 08:50:10PM +0200, Greg KH wrote:
> > exfat is just a random old code drop from Samsung hastily picked up,
> > and also duplicating the fat16/32 functionality, and without
> > consultation of the developes of that who are trying to work through
> > their process of contributing the uptodate and official version of it.
> 
> Those developers are still yet to be found, we only got a "drop" of the
> samsung code _after_ this code was merged, from non-samsung people.  No
> samsung developers have said they would actually help with any of this
> work, and when asked what differed from the in-tree stuff, I got a list,
> but no specifics.  I'll be working through that list over the next month
> to resolve those issues.

Park offered to help with a new version, and the Samsung guys are going
through their internal review process to work upstream it.  Remember
it is their codebase in each of the variants here.  While it is fine
if we take some code that has been abandoned by the original developers
and still merge it with helping hands I think it is very rude to not
at least wait for them to get their act of working with their corporate
overlords together first.  It isn't like we've been waiting forever
for an exfat driver - the patent grant has been extremely recent, and
this whole thing just seems like a publicity stunt to be honest.  Give
them a couple weeks to sort their stuff out and then we can decide
how to proceed.  I for one would defintively prefer to have code
maintained by the original maintainers if possible.  And not have
them hindered by the staging process to work on their own code.

> To get back to the meta-problem here, we need a common "vfs/filesystem"
> tree somewhere with reviewers.  I'm glad to set up the tree and handle
> patches, but I am not a vfs expert by any means (I only understand the
> virtual half, not the backing half), so I can't be the only one to do
> reviews.  Do you know of anyone that we can drag in as reviewers to help
> make it easier for new filesystems to get reviews as well as existing
> ones to have their patches collected and forwarded on to Linus at the
> proper times?

Following how staging works and its arcane rules I don't want it to be
anywhere near fs code.  And seriously, it is not like we need one
magic tree to deal with all file systems.  The whole point of git is
that people can setup a tree to collaborate wherever they want if you
just find people to work on it.  And we've had tons of successful
drivers and filesystems that worked that way.  And at least the ones
I've followed seem a lot more productive than the staging show that
is baѕed around coding style cleanup micropatches.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v17 0/1] staging: Add VirtualBox guest shared folder (vboxsf) support

[Christoph Hellwig]

On Mon, Oct 28, 2019 at 12:17:43PM +0100, Hans de Goede wrote:
> Hi Greg,
> 
> As discussed previously can you please take vboxsf upstream through
> drivers/staging?
> 
> It has seen many revisions on the fsdevel list, but it seems that the
> fsdevel people are to busy to pick it up.
> 
> Previous versions of this patch have been reviewed by Al Viro, David Howells
> and Christoph Hellwig (all in the Cc) and I believe that the current
> version addresses all their review remarks.

Please just send it to Linus directly.  This is the equivalent of
consumer hardware enablement and it is in a state as clean as it gets
for the rather messed up protocol.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 10/10] errno.h: Provide EFSCORRUPTED for everybody

[Christoph Hellwig]

On Sun, Nov 03, 2019 at 08:45:06PM -0500, Valdis Kletnieks wrote:
> There's currently 6 filesystems that have the same #define. Move it
> into errno.h so it's defined in just one place.

And 4 out of 6 also define EFSBADCRC, so please lift that as well.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [GIT PULL] IIO fixes / Staging driver for 5.4-rc7

[Christoph Hellwig]

On Sun, Nov 10, 2019 at 04:43:03PM +0100, Greg KH wrote:
> The staging driver addition is the vboxsf filesystem, which is the
> VirtualBox guest shared folder code.  Hans has been trying to get
> filesystem reviewers to review the code for many months now, and
> Christoph finally said to just merge it in staging now as it is
> stand-alone and the filesystem people can review it easier over time
> that way.

No, this is absolutely contrary to what I said.  I told Hans to just
send it to Linus because it is ready and not staging fodder a atll.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [GIT PULL] Staging driver patches for 3.19-rc1

[Christoph Hellwig]

On Mon, Dec 15, 2014 at 07:23:35PM +0100, Richard Weinberger wrote:
> I don't understand this kind of logic.
> a) Binder is considered a piece of shite.
> b) Google is working on a (hopefully sane) replacement.
> 
> Why moving it out of staging then? What is the benefit?

There is none, and Greg didn't even bother addressing the various
comments when this first came up.

So a clear NAK from me on this one.  

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [GIT PULL] Staging driver patches for 3.19-rc1

[Christoph Hellwig]

On Mon, Dec 15, 2014 at 10:56:38AM -0800, Greg KH wrote:
> Ok, that was a bit snotty on my part, I apologize.
> 
> But really, this is self-contained, doesn't touch any core
> infrastructure, and is really just like any other driver for hardware
> that people don't use.  It shouldn't affect anything elsewhere in the
> kernel, so objecting to it seems odd to me.

I does dig into internals in quite nasty ways.  I driver that is always
modular and just uses existing exported APIs in proper ways and adds no
to little new userspaces ABIs is one things.  A new IPCs mechanisms that
pokes into internals and exposes a huge ABI/API is a totally different
thing.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] SCSI:STORVSC Use SCSI layer to allocate memory for per-command device request data

[Christoph Hellwig]

STORVSC uses its own momory pool to manage device request data. However, SCSI 
layer already has a mechanisim for allocating additional memory for each 
command issued to device driver. This patch removes the memory pool in STORVSC 
and makes it use SCSI layer to allocate memory for device request data.

Reviewed-by: Long Li 
Signed-off-by: Christoph Hellwig 
---
 drivers/scsi/storvsc_drv.c | 119 +++--
 1 file changed, 8 insertions(+), 111 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 4cff0dd..14ee98e 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -32,7 +32,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -309,14 +308,6 @@ enum storvsc_request_type {
  * This is the end of Protocol specific defines.
  */
 
-
-/*
- * We setup a mempool to allocate request structures for this driver
- * on a per-lun basis. The following define specifies the number of
- * elements in the pool.
- */
-
-#define STORVSC_MIN_BUF_NR 64
 static int storvsc_ringbuffer_size = (20 * PAGE_SIZE);
 
 module_param(storvsc_ringbuffer_size, int, S_IRUGO);
@@ -346,7 +337,6 @@ static void storvsc_on_channel_callback(void *context);
 #define STORVSC_IDE_MAX_CHANNELS   1
 
 struct storvsc_cmd_request {
-   struct list_head entry;
struct scsi_cmnd *cmd;
 
unsigned int bounce_sgl_count;
@@ -357,7 +347,6 @@ struct storvsc_cmd_request {
/* Synchronize the request/response if needed */
struct completion wait_event;
 
-   unsigned char *sense_buffer;
struct hv_multipage_buffer data_buffer;
struct vstor_packet vstor_packet;
 };
@@ -389,11 +378,6 @@ struct storvsc_device {
struct storvsc_cmd_request reset_request;
 };
 
-struct stor_mem_pools {
-   struct kmem_cache *request_pool;
-   mempool_t *request_mempool;
-};
-
 struct hv_host_device {
struct hv_device *dev;
unsigned int port;
@@ -1070,10 +1054,8 @@ static void storvsc_command_completion(struct 
storvsc_cmd_request *cmd_request)
 {
struct scsi_cmnd *scmnd = cmd_request->cmd;
struct hv_host_device *host_dev = shost_priv(scmnd->device->host);
-   void (*scsi_done_fn)(struct scsi_cmnd *);
struct scsi_sense_hdr sense_hdr;
struct vmscsi_request *vm_srb;
-   struct stor_mem_pools *memp = scmnd->device->hostdata;
struct Scsi_Host *host;
struct storvsc_device *stor_dev;
struct hv_device *dev = host_dev->dev;
@@ -1109,14 +1091,7 @@ static void storvsc_command_completion(struct 
storvsc_cmd_request *cmd_request)
cmd_request->data_buffer.len -
vm_srb->data_transfer_length);
 
-   scsi_done_fn = scmnd->scsi_done;
-
-   scmnd->host_scribble = NULL;
-   scmnd->scsi_done = NULL;
-
-   scsi_done_fn(scmnd);
-
-   mempool_free(cmd_request, memp->request_mempool);
+   scmnd->scsi_done(scmnd);
 }
 
 static void storvsc_on_io_completion(struct hv_device *device,
@@ -1160,7 +1135,7 @@ static void storvsc_on_io_completion(struct hv_device 
*device,
SRB_STATUS_AUTOSENSE_VALID) {
/* autosense data available */
 
-   memcpy(request->sense_buffer,
+   memcpy(request->cmd->sense_buffer,
   vstor_packet->vm_srb.sense_data,
   vstor_packet->vm_srb.sense_info_length);
 
@@ -1378,55 +1353,6 @@ static int storvsc_do_io(struct hv_device *device,
return ret;
 }
 
-static int storvsc_device_alloc(struct scsi_device *sdevice)
-{
-   struct stor_mem_pools *memp;
-   int number = STORVSC_MIN_BUF_NR;
-
-   memp = kzalloc(sizeof(struct stor_mem_pools), GFP_KERNEL);
-   if (!memp)
-   return -ENOMEM;
-
-   memp->request_pool =
-   kmem_cache_create(dev_name(&sdevice->sdev_dev),
-   sizeof(struct storvsc_cmd_request), 0,
-   SLAB_HWCACHE_ALIGN, NULL);
-
-   if (!memp->request_pool)
-   goto err0;
-
-   memp->request_mempool = mempool_create(number, mempool_alloc_slab,
-   mempool_free_slab,
-   memp->request_pool);
-
-   if (!memp->request_mempool)
-   goto err1;
-
-   sdevice->hostdata = memp;
-
-   return 0;
-
-err1:
-   kmem_cache_destroy(memp->request_pool);
-
-err0:
-   kfree(memp);
-   return -ENOMEM;
-}
-
-static void storvsc_device_destroy(struct scsi_device *sdevice)
-{
-   struct stor_mem_pools *memp = sdevice->hostdata;
-
-   if (!memp)
-   return;
-
-   mempool_destroy(memp->request_mempool);
-   kmem_cache_destroy(memp

Re: [PATCH 0/4] Drivers: scsi: storvsc: Fix miscellaneous issues

[Christoph Hellwig]

Thanks, applied to scsi-for-3.20.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: erofs: fix unexpected out-of-bound data access

[Christoph Hellwig]

> +++ b/drivers/staging/erofs/data.c
> @@ -304,7 +304,7 @@ static inline struct bio *erofs_read_raw_page(struct bio 
> *bio,
>   *last_block = current_block;
>  
>   /* shift in advance in case of it followed by too many gaps */
> - if (unlikely(bio->bi_vcnt >= bio->bi_max_vecs)) {
> + if (bio->bi_iter.bi_size >= bio->bi_max_vecs * PAGE_SIZE) {

This is still a very odd check.  bi_max_vecs * PAGE_SIZE is rather
arbitrary… and more importantly bi_max_vecs is not really a public
field, in fact this is the only place every using it outside the
core block layer.

I think the logic in this function should be reworked to what we
do elsewhere in the kernel, that is just add to the bio until
bio_add_page fails, in which case you submit the bio and start
a new one.  Then once you are done with your operation just submit
the bio.  Which unless I'm missing something is what the code does,
except for the goto loop obsfucation that is trying to hide it.

So why not something like:


diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
index 0714061ba888..122714e19079 100644
--- a/drivers/staging/erofs/data.c
+++ b/drivers/staging/erofs/data.c
@@ -296,20 +296,9 @@ static inline struct bio *erofs_read_raw_page(struct bio 
*bio,
}
}
 
-   err = bio_add_page(bio, page, PAGE_SIZE, 0);
-   /* out of the extent or bio is full */
-   if (err < PAGE_SIZE)
+   if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE)
goto submit_bio_retry;
-
*last_block = current_block;
-
-   /* shift in advance in case of it followed by too many gaps */
-   if (unlikely(bio->bi_vcnt >= bio->bi_max_vecs)) {
-   /* err should reassign to 0 after submitting */
-   err = 0;
-   goto submit_bio_out;
-   }
-
return bio;
 
 err_out:
@@ -323,9 +312,7 @@ static inline struct bio *erofs_read_raw_page(struct bio 
*bio,
 
/* if updated manually, continuous pages has a gap */
if (bio)
-submit_bio_out:
__submit_bio(bio, REQ_OP_READ, 0);
-
return unlikely(err) ? ERR_PTR(err) : NULL;
 }
 
@@ -387,8 +374,7 @@ static int erofs_raw_access_readpages(struct file *filp,
}
DBG_BUGON(!list_empty(pages));
 
-   /* the rare case (end in gaps) */
-   if (unlikely(bio))
+   if (bio)
__submit_bio(bio, REQ_OP_READ, 0);
return 0;
 }

>   /* err should reassign to 0 after submitting */
>   err = 0;
>   goto submit_bio_out;
> -- 
> 2.17.1
> 
---end quoted text---
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: erofs: fix unexpected out-of-bound data access

[Christoph Hellwig]

On Fri, Apr 12, 2019 at 08:06:33AM -0700, Christoph Hellwig wrote:
> > +++ b/drivers/staging/erofs/data.c
> > @@ -304,7 +304,7 @@ static inline struct bio *erofs_read_raw_page(struct 
> > bio *bio,
> > *last_block = current_block;
> >  
> > /* shift in advance in case of it followed by too many gaps */
> > -   if (unlikely(bio->bi_vcnt >= bio->bi_max_vecs)) {
> > +   if (bio->bi_iter.bi_size >= bio->bi_max_vecs * PAGE_SIZE) {
> 
> This is still a very odd check.  bi_max_vecs * PAGE_SIZE is rather
> arbitrary… and more importantly bi_max_vecs is not really a public
> field, in fact this is the only place every using it outside the
> core block layer.
> 
> I think the logic in this function should be reworked to what we
> do elsewhere in the kernel, that is just add to the bio until
> bio_add_page fails, in which case you submit the bio and start
> a new one.  Then once you are done with your operation just submit
> the bio.  Which unless I'm missing something is what the code does,
> except for the goto loop obsfucation that is trying to hide it.
> 
> So why not something like:

And looking at this a little more - I think you should drop the
erofs raw ops entirely.  The iomap-based readpage and readpages
from fs/iomap.c should serve your needs just fine without all that
duplication.  The only thing missing is the cleancache calls,
which could easily be added.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 01/16] media: videobuf-dma-contig: use dma_mmap_coherent

[Christoph Hellwig]

dma_alloc_coherent does not return a physical address, but a DMA
address, which might be remapped or have an offset.  Passing this
DMA address to vm_iomap_memory is completely bogus.  Use the proper
dma_mmap_coherent helper instead, and stop passing __GFP_COMP
to dma_alloc_coherent, as the memory management inside the DMA
allocator is hidden from the callers.

Fixes: a8f3c203e19b ("[media] videobuf-dma-contig: add cache support")
Signed-off-by: Christoph Hellwig 
---
 drivers/media/v4l2-core/videobuf-dma-contig.c | 23 +++
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf-dma-contig.c 
b/drivers/media/v4l2-core/videobuf-dma-contig.c
index e1bf50df4c70..a5942ea38f1f 100644
--- a/drivers/media/v4l2-core/videobuf-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf-dma-contig.c
@@ -39,11 +39,11 @@ struct videobuf_dma_contig_memory {
 
 static int __videobuf_dc_alloc(struct device *dev,
   struct videobuf_dma_contig_memory *mem,
-  unsigned long size, gfp_t flags)
+  unsigned long size)
 {
mem->size = size;
-   mem->vaddr = dma_alloc_coherent(dev, mem->size,
-   &mem->dma_handle, flags);
+   mem->vaddr = dma_alloc_coherent(dev, mem->size, &mem->dma_handle,
+   GFP_KERNEL);
 
if (!mem->vaddr) {
dev_err(dev, "memory alloc size %ld failed\n", mem->size);
@@ -260,8 +260,7 @@ static int __videobuf_iolock(struct videobuf_queue *q,
return videobuf_dma_contig_user_get(mem, vb);
 
/* allocate memory for the read() method */
-   if (__videobuf_dc_alloc(q->dev, mem, PAGE_ALIGN(vb->size),
-   GFP_KERNEL))
+   if (__videobuf_dc_alloc(q->dev, mem, PAGE_ALIGN(vb->size)))
return -ENOMEM;
break;
case V4L2_MEMORY_OVERLAY:
@@ -280,7 +279,6 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
struct videobuf_dma_contig_memory *mem;
struct videobuf_mapping *map;
int retval;
-   unsigned long size;
 
dev_dbg(q->dev, "%s\n", __func__);
 
@@ -298,23 +296,18 @@ static int __videobuf_mmap_mapper(struct videobuf_queue 
*q,
BUG_ON(!mem);
MAGIC_CHECK(mem->magic, MAGIC_DC_MEM);
 
-   if (__videobuf_dc_alloc(q->dev, mem, PAGE_ALIGN(buf->bsize),
-   GFP_KERNEL | __GFP_COMP))
+   if (__videobuf_dc_alloc(q->dev, mem, PAGE_ALIGN(buf->bsize)))
goto error;
 
-   /* Try to remap memory */
-   size = vma->vm_end - vma->vm_start;
-   vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
-
/* the "vm_pgoff" is just used in v4l2 to find the
 * corresponding buffer data structure which is allocated
 * earlier and it does not mean the offset from the physical
 * buffer start address as usual. So set it to 0 to pass
-* the sanity check in vm_iomap_memory().
+* the sanity check in dma_mmap_coherent().
 */
vma->vm_pgoff = 0;
-
-   retval = vm_iomap_memory(vma, mem->dma_handle, size);
+   retval = dma_mmap_coherent(q->dev, vma, mem->vaddr, mem->dma_handle,
+   vma->vm_end - vma->vm_start);
if (retval) {
dev_err(q->dev, "mmap: remap failed with error %d. ",
retval);
-- 
2.20.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 02/16] drm/ati_pcigart: stop using drm_pci_alloc

[Christoph Hellwig]

Remove usage of the legacy drm PCI DMA wrappers, and with that the
incorrect usage cocktail of __GFP_COMP, virt_to_page on DMA allocation
and SetPageReserved.

Signed-off-by: Christoph Hellwig 
---
 drivers/gpu/drm/ati_pcigart.c | 27 +++
 include/drm/ati_pcigart.h |  5 -
 2 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/ati_pcigart.c b/drivers/gpu/drm/ati_pcigart.c
index 2362f07fe1fc..f66d4fccd812 100644
--- a/drivers/gpu/drm/ati_pcigart.c
+++ b/drivers/gpu/drm/ati_pcigart.c
@@ -41,21 +41,14 @@
 static int drm_ati_alloc_pcigart_table(struct drm_device *dev,
   struct drm_ati_pcigart_info *gart_info)
 {
-   gart_info->table_handle = drm_pci_alloc(dev, gart_info->table_size,
-   PAGE_SIZE);
-   if (gart_info->table_handle == NULL)
+   gart_info->table_vaddr = dma_alloc_coherent(&dev->pdev->dev,
+   gart_info->table_size, &gart_info->table_handle,
+   GFP_KERNEL);
+   if (!gart_info->table_vaddr)
return -ENOMEM;
-
return 0;
 }
 
-static void drm_ati_free_pcigart_table(struct drm_device *dev,
-  struct drm_ati_pcigart_info *gart_info)
-{
-   drm_pci_free(dev, gart_info->table_handle);
-   gart_info->table_handle = NULL;
-}
-
 int drm_ati_pcigart_cleanup(struct drm_device *dev, struct 
drm_ati_pcigart_info *gart_info)
 {
struct drm_sg_mem *entry = dev->sg;
@@ -87,8 +80,10 @@ int drm_ati_pcigart_cleanup(struct drm_device *dev, struct 
drm_ati_pcigart_info
}
 
if (gart_info->gart_table_location == DRM_ATI_GART_MAIN &&
-   gart_info->table_handle) {
-   drm_ati_free_pcigart_table(dev, gart_info);
+   gart_info->table_vaddr) {
+   dma_free_coherent(&dev->pdev->dev, gart_info->table_size,
+   gart_info->table_vaddr, 
gart_info->table_handle);
+   gart_info->table_vaddr = NULL;
}
 
return 1;
@@ -127,9 +122,9 @@ int drm_ati_pcigart_init(struct drm_device *dev, struct 
drm_ati_pcigart_info *ga
goto done;
}
 
-   pci_gart = gart_info->table_handle->vaddr;
-   address = gart_info->table_handle->vaddr;
-   bus_address = gart_info->table_handle->busaddr;
+   pci_gart = gart_info->table_vaddr;
+   address = gart_info->table_vaddr;
+   bus_address = gart_info->table_handle;
} else {
address = gart_info->addr;
bus_address = gart_info->bus_addr;
diff --git a/include/drm/ati_pcigart.h b/include/drm/ati_pcigart.h
index a728a1364e66..2ffe278ba3b3 100644
--- a/include/drm/ati_pcigart.h
+++ b/include/drm/ati_pcigart.h
@@ -18,7 +18,10 @@ struct drm_ati_pcigart_info {
void *addr;
dma_addr_t bus_addr;
dma_addr_t table_mask;
-   struct drm_dma_handle *table_handle;
+
+   dma_addr_t table_handle;
+   void *table_vaddr;
+
struct drm_local_map mapping;
int table_size;
 };
-- 
2.20.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel