Re: RFC: prepare for struct scatterlist entries without page backing
Hi Christoph, On Fri, Aug 14, 2015 at 12:35 AM, Christoph Hellwig wrote: > On Thu, Aug 13, 2015 at 09:37:37AM +1000, Julian Calaby wrote: >> I.e. ~90% of this patch set seems to be just mechanically dropping >> BUG_ON()s and converting open coded stuff to use accessor functions >> (which should be macros or get inlined, right?) - and the remaining >> bit is not flushing if we don't have a physical page somewhere. > > Which is was 90%. By lines changed most actually is the diffs for > the cache flushing. I was talking in terms of changes made, not lines changed: by my recollection, about a third of the patches didn't touch flush calls and most of the lines changed looked like refactoring so that making the flush call conditional would be easier. I guess it smelled like you were doing lots of distinct changes in a single patch and I got my numbers wrong. >> Would it make sense to split this patch set into a few bits: one to >> drop all the useless BUG_ON()s, one to convert all the open coded >> stuff to accessor functions, then another to do the actual page-less >> sg stuff? > > Without the ifs the BUG_ON() actually are useful to assert we > never feed the sort of physical addresses we can't otherwise support, > so I don't think that part is doable. My point is that there's a couple of patches that only remove BUG_ON()s, which implies that for that particular driver it doesn't matter if there's a physical page or not, so therefore that code is purely "documentation". Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC: prepare for struct scatterlist entries without page backing
Hi, On Wed, Aug 12, 2015 at 10:42 PM, Boaz Harrosh wrote: > On 08/12/2015 10:05 AM, Christoph Hellwig wrote: >> It turns out most DMA mapping implementation can handle SGLs without >> page structures with some fairly simple mechanical work. Most of it >> is just about consistently using sg_phys. For implementations that >> need to flush caches we need a new helper that skips these cache >> flushes if a entry doesn't have a kernel virtual address. >> >> However the ccio (parisc) and sba_iommu (parisc & ia64) IOMMUs seem >> to be operate mostly on virtual addresses. It's a fairly odd concept >> that I don't fully grasp, so I'll need some help with those if we want >> to bring this forward. >> >> Additional this series skips ARM entirely for now. The reason is >> that most arm implementations of the .map_sg operation just iterate >> over all entries and call ->map_page for it, which means we'd need >> to convert those to a ->map_pfn similar to Dan's previous approach. >> > [snip] > > It is a bit of work but is worth while, and accelerating tremendously > lots of workloads. Not like this abomination which only branches > things more and more, and making things fatter and slower. As a random guy reading a big bunch of patches on code I know almost nothing about, parts of this comment really resonated with me: overall, we seem to be adding a lot of if statements to code that appears to be in a hot path. I.e. ~90% of this patch set seems to be just mechanically dropping BUG_ON()s and converting open coded stuff to use accessor functions (which should be macros or get inlined, right?) - and the remaining bit is not flushing if we don't have a physical page somewhere. Would it make sense to split this patch set into a few bits: one to drop all the useless BUG_ON()s, one to convert all the open coded stuff to accessor functions, then another to do the actual page-less sg stuff? Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-sunxi] [PATCH 4/9] rc: sunxi-cir: Add support for an optional reset controller
Hi Hans,
On Fri, Nov 21, 2014 at 2:55 AM, Hans de Goede wrote:
> On sun6i the cir block is attached to the reset controller, add support
> for de-asserting the reset if a reset controller is specified in dt.
>
> Signed-off-by: Hans de Goede
> ---
> drivers/media/rc/sunxi-cir.c | 25 +++--
> 1 file changed, 23 insertions(+), 2 deletions(-)
Shouldn't we be updating the binding documentation?
> diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
> index bcee8e1..895fb65 100644
> --- a/drivers/media/rc/sunxi-cir.c
> +++ b/drivers/media/rc/sunxi-cir.c
> @@ -23,6 +23,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #define SUNXI_IR_DEV "sunxi-ir"
> @@ -95,6 +96,7 @@ struct sunxi_ir {
> int irq;
> struct clk *clk;
> struct clk *apb_clk;
> + struct reset_control *rst;
> const char *map_name;
> };
>
> @@ -166,15 +168,29 @@ static int sunxi_ir_probe(struct platform_device *pdev)
> return PTR_ERR(ir->clk);
> }
>
> + /* Reset (optional) */
> + ir->rst = devm_reset_control_get_optional(dev, NULL);
> + if (IS_ERR(ir->rst)) {
> + ret = PTR_ERR(ir->rst);
> + if (ret == -EPROBE_DEFER)
> + return ret;
> + ir->rst = NULL;
> + } else {
> + ret = reset_control_deassert(ir->rst);
> + if (ret)
> + return ret;
> + }
> +
> ret = clk_set_rate(ir->clk, SUNXI_IR_BASE_CLK);
> if (ret) {
> dev_err(dev, "set ir base clock failed!\n");
> - return ret;
> + goto exit_reset_assert;
> }
>
> if (clk_prepare_enable(ir->apb_clk)) {
> dev_err(dev, "try to enable apb_ir_clk failed\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto exit_reset_assert;
> }
>
> if (clk_prepare_enable(ir->clk)) {
> @@ -271,6 +287,9 @@ exit_clkdisable_clk:
> clk_disable_unprepare(ir->clk);
> exit_clkdisable_apb_clk:
> clk_disable_unprepare(ir->apb_clk);
> +exit_reset_assert:
> + if (ir->rst)
> + reset_control_assert(ir->rst);
>
> return ret;
> }
> @@ -282,6 +301,8 @@ static int sunxi_ir_remove(struct platform_device *pdev)
>
> clk_disable_unprepare(ir->clk);
> clk_disable_unprepare(ir->apb_clk);
> + if (ir->rst)
> + reset_control_assert(ir->rst);
>
> spin_lock_irqsave(&ir->ir_lock, flags);
> /* disable IR IRQ */
> --
> 2.1.0
>
> --
> You received this message because you are subscribed to the Google Groups
> "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to linux-sunxi+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/22] Add and use pci_zalloc_consistent
Hi Joe, On Tue, Jun 24, 2014 at 5:13 AM, Joe Perches wrote: > On Mon, 2014-06-23 at 10:25 -0700, Luis R. Rodriguez wrote: >> On Mon, Jun 23, 2014 at 06:41:28AM -0700, Joe Perches wrote: >> > Adding the helper reduces object code size as well as overall >> > source size line count. >> > >> > It's also consistent with all the various zalloc mechanisms >> > in the kernel. >> > >> > Done with a simple cocci script and some typing. >> >> Awesome, any chance you can paste in the SmPL? Also any chance >> we can get this added to a make coccicheck so that maintainers >> moving forward can use that to ensure that no new code is >> added that uses the old school API? > > Not many of these are recent. > > Arnd Bergmann reasonably suggested that the pci_alloc_consistent > api be converted the the more widely used dma_alloc_coherent. > > https://lkml.org/lkml/2014/6/23/513 > >> Shouldn't these drivers just use the normal dma-mapping API now? > > and I replied: > > https://lkml.org/lkml/2014/6/23/525 > >> Maybe. I wouldn't mind. >> They do seem to have a trivial bit of unnecessary overhead for >> hwdev == NULL ? NULL : &hwdev->dev > > Anyway, here's the little script. > I'm not sure it's worthwhile to add it though. > > $ cat ./scripts/coccinelle/api/alloc/pci_zalloc_consistent.cocci > /// > /// Use pci_zalloc_consistent rather than > /// pci_alloc_consistent followed by memset with 0 > /// > /// This considers some simple cases that are common and easy to validate > /// Note in particular that there are no ...s in the rule, so all of the > /// matched code has to be contiguous > /// > /// Blatantly cribbed from: scripts/coccinelle/api/alloc/kzalloc-simple.cocci > > @@ > type T, T2; > expression x; > expression E1,E2,E3; > statement S; > @@ > > - x = (T)pci_alloc_consistent(E1,E2,E3); > + x = pci_zalloc_consistent(E1,E2,E3); > if ((x==NULL) || ...) S > - memset((T2)x,0,E2); I don't know much about SmPL, but wouldn't having that if statement there reduce your matches? Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/ .Plan: http://sites.google.com/site/juliancalaby/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
