Re: [PATCH] serial: sirf: Simplify a test
On Tue, 8 Nov 2016, Christophe JAILLET wrote: > Le 07/11/2016 à 14:50, Arnd Bergmann a écrit : > > On Tuesday, November 1, 2016 8:03:33 AM CET Christophe JAILLET wrote: > > > 'dmaengine_prep_dma_cyclic()' does not return an error pointer, so the > > > test > > > can be simplified to be more consistent. > > > > > > Signed-off-by: Christophe JAILLET > > The change looks correct in principle. It would be good to automate looking > > for other instances of this bug. How did you find it? Do you have e.g. a > > coccinelle script or did you just stumble over the issue by accident? > Hi, > > You can have an idea of the strategy used in this post: >https://lkml.org/lkml/2016/11/1/388 > > Julia is currently working on a more complete strategy in order to find such > issues. I have a set of reports in the files listed below. If anyone wants to take a look at them, I would be happy to send them along. It is definitely possible that the list contains false positives. julia arch/arm64/kernel/ptrace.c arch/arm/mach-davinci/usb-da8xx.c arch/arm/mach-omap2/pm.c arch/mips/ath79/clock.c arch/mips/ath79/pci.c arch/sh/boards/mach-sdk7786/setup.c arch/sh/kernel/cpu/clock.c arch/sh/kernel/cpu/sh2a/clock-sh7264.c arch/sh/kernel/cpu/sh2a/clock-sh7269.c arch/sh/kernel/cpu/sh4a/clock-sh7343.c arch/sh/kernel/cpu/sh4a/clock-sh7366.c arch/sh/kernel/cpu/sh4a/clock-sh7722.c arch/sh/kernel/cpu/sh4a/clock-sh7723.c arch/sh/kernel/cpu/sh4a/clock-sh7724.c arch/sh/kernel/cpu/sh4a/clock-sh7734.c arch/sh/kernel/cpu/sh4a/clock-sh7757.c arch/sh/kernel/cpu/sh4a/clock-sh7785.c arch/sh/kernel/cpu/sh4a/clock-sh7786.c arch/sh/kernel/cpu/sh4a/clock-shx3.c arch/sparc/kernel/perf_event.c block/bsg.c drivers/acpi/apei/ghes.c drivers/block/loop.c drivers/block/mtip32xx/mtip32xx.c drivers/block/nbd.c drivers/block/pktcdvd.c drivers/clk/bcm/clk-bcm2835.c drivers/clk/keystone/pll.c drivers/clk/mmp/clk-frac.c drivers/clk/renesas/renesas-cpg-mssr.c drivers/clk/rockchip/clk.c drivers/clk/sirf/clk-atlas6.c drivers/clk/sirf/clk-atlas7.c drivers/clk/sirf/clk-prima2.c drivers/clk/spear/clk-aux-synth.c drivers/clk/spear/clk-frac-synth.c drivers/clk/spear/clk-gpt-synth.c drivers/clk/spear/clk-vco-pll.c drivers/clk/ti/fapll.c drivers/clk/ux500/clk-prcc.c drivers/clk/ux500/clk-prcmu.c drivers/clocksource/armv7m_systick.c drivers/cpufreq/loongson1-cpufreq.c drivers/cpufreq/spear-cpufreq.c drivers/crypto/marvell/hash.c drivers/crypto/sunxi-ss/sun4i-ss-core.c drivers/dma/dmaengine.c drivers/dma/imx-sdma.c drivers/dma/xilinx/xilinx_dma.c drivers/gpio/gpiolib.c drivers/gpu/drm/drm_edid.c drivers/gpu/drm/drm_gem.c drivers/gpu/drm/drm_property.c drivers/gpu/drm/etnaviv/etnaviv_dump.c drivers/gpu/drm/i915/i915_gem_gtt.c drivers/gpu/drm/i915/i915_gem_userptr.c drivers/gpu/drm/i915/intel_display.c drivers/gpu/drm/i915/intel_engine_cs.c drivers/gpu/drm/i915/intel_fbdev.c drivers/gpu/drm/i915/intel_guc_loader.c drivers/gpu/drm/i915/intel_overlay.c drivers/gpu/drm/i915/intel_ringbuffer.c drivers/gpu/drm/imx/imx-drm-core.c drivers/gpu/drm/imx/ipuv3-crtc.c drivers/gpu/drm/msm/dsi/dsi.c drivers/gpu/drm/msm/dsi/dsi_manager.c drivers/gpu/drm/msm/edp/edp.c drivers/gpu/drm/msm/hdmi/hdmi.c drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c drivers/gpu/drm/msm/msm_drv.c drivers/gpu/drm/msm/msm_fbdev.c drivers/gpu/drm/nouveau/nvkm/subdev/clk/nv50.c drivers/gpu/drm/omapdrm/omap_dmm_tiler.c drivers/gpu/drm/omapdrm/omap_fbdev.c drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c drivers/gpu/drm/tegra/dc.c drivers/gpu/drm/tegra/dpaux.c drivers/gpu/drm/vgem/vgem_fence.c drivers/gpu/drm/vmwgfx/vmwgfx_context.c drivers/gpu/drm/vmwgfx/vmwgfx_drv.c drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c drivers/gpu/drm/vmwgfx/vmwgfx_fifo.c drivers/iio/adc/rockchip_saradc.c drivers/infiniband/core/agent.c drivers/infiniband/core/ucma.c drivers/infiniband/core/ucm.c drivers/infiniband/core/user_mad.c drivers/infiniband/hw/hfi1/user_sdma.c drivers/infiniband/hw/mlx4/cm.c drivers/infiniband/hw/mlx4/mcg.c drivers/infiniband/hw/mlx5/main.c drivers/infiniband/hw/mthca/mthca_cmd.c drivers/infiniband/hw/qedr/verbs.c drivers/infiniband/hw/usnic/usnic_ib_main.c drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c drivers/infiniband/hw/usnic/usnic_ib_verbs.c drivers/infiniband/ulp/ipoib/ipoib_main.c drivers/input/keyboard/gpio_keys.c drivers/iommu/ipmmu-vmsa.c drivers/leds/leds-pca963x.c drivers/lightnvm/core.c drivers/md/dm-cache-policy.c drivers/md/dm-mpath.c drivers/media/platform/sti/hva/hva-hw.c drivers/media/rc/sunxi-cir.c drivers/media/v4l2-core/videobuf2-core.c drivers/mfd/max8997.c drivers/mfd/max8998.c drivers/mfd/twl-core.c drivers/misc/c2port/c2port-duramar2150.c drivers/misc/cxl/api.c drivers/misc/cxl/pci.c drivers/misc/cxl/phb.c drivers/misc/mic/scif/scif_dma.c drivers/mmc/host/omap_hsmmc.c drivers/mmc/host/sdhci-st.c drivers/mtd/devices/docg3.c drivers/mtd/nand/davinci_nand.c drivers/mtd/nand/nand_base.c drivers/mtd/spi-nor/spi-nor.c drivers/net/can/usb/gs_usb.c drivers/net/ethernet/amd/sun3lance.c drivers/net/ethernet/broadc
Re: [PATCH] serial: sirf: Simplify a test
On Tuesday, November 8, 2016 8:18:04 AM CET Christophe JAILLET wrote: > Le 07/11/2016 à 14:50, Arnd Bergmann a écrit : > > On Tuesday, November 1, 2016 8:03:33 AM CET Christophe JAILLET wrote: > >> 'dmaengine_prep_dma_cyclic()' does not return an error pointer, so the test > >> can be simplified to be more consistent. > >> > >> Signed-off-by: Christophe JAILLET > > The change looks correct in principle. It would be good to automate looking > > for other instances of this bug. How did you find it? Do you have e.g. a > > coccinelle script or did you just stumble over the issue by accident? > Hi, > > You can have an idea of the strategy used in this post: > https://lkml.org/lkml/2016/11/1/388 > > Julia is currently working on a more complete strategy in order to find > such issues. Ah, very nice! Arnd
Re: [PATCH] serial: sirf: Simplify a test
Le 07/11/2016 à 14:50, Arnd Bergmann a écrit : On Tuesday, November 1, 2016 8:03:33 AM CET Christophe JAILLET wrote: 'dmaengine_prep_dma_cyclic()' does not return an error pointer, so the test can be simplified to be more consistent. Signed-off-by: Christophe JAILLET The change looks correct in principle. It would be good to automate looking for other instances of this bug. How did you find it? Do you have e.g. a coccinelle script or did you just stumble over the issue by accident? Hi, You can have an idea of the strategy used in this post: https://lkml.org/lkml/2016/11/1/388 Julia is currently working on a more complete strategy in order to find such issues. In the meantime, if you want, I can send the coccinelle scripts used. Best regards, CJ
Re: [PATCH] serial: sirf: Simplify a test
On Mon, 7 Nov 2016, Arnd Bergmann wrote:
> On Tuesday, November 1, 2016 8:03:33 AM CET Christophe JAILLET wrote:
> > 'dmaengine_prep_dma_cyclic()' does not return an error pointer, so the test
> > can be simplified to be more consistent.
> >
> > Signed-off-by: Christophe JAILLET
>
> The change looks correct in principle. It would be good to automate looking
> for other instances of this bug. How did you find it? Do you have e.g. a
> coccinelle script or did you just stumble over the issue by accident?
I'm working on collecting this information in a more general way. It is
complicated by the fact that there are some functions that have the same
names but different behaviors, and I want to be clear about when that is
an issue. There are nevertheless limits to the accuracy that can be
obtained with Coccinelle, because Coccinelle doesn't take values into
account. Sometimes a variable is initialized to NULL, just to have a
starting value, but in practice the only way to reach an error return is
via conditionals that have the effect of ensuring that the value is
ERR_PTR. So at least the cases that are reported as being able to return
both NULL and ERR_PTR will need some careful checking.
julia
>
> There is one problem with your patch:
>
> > drivers/tty/serial/sirfsoc_uart.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/sirfsoc_uart.c
> > b/drivers/tty/serial/sirfsoc_uart.c
> > index b186c9c4f850..666ca3156961 100644
> > --- a/drivers/tty/serial/sirfsoc_uart.c
> > +++ b/drivers/tty/serial/sirfsoc_uart.c
> > @@ -609,7 +609,7 @@ static void sirfsoc_uart_start_next_rx_dma(struct
> > uart_port *port)
> > sirfport->rx_dma_items.dma_addr, SIRFSOC_RX_DMA_BUF_SIZE,
> > SIRFSOC_RX_DMA_BUF_SIZE / 2,
> > DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT);
> > - if (IS_ERR_OR_NULL(sirfport->rx_dma_items.desc)) {
> > + if (!sirfport->rx_dma_items.desc) {
> > dev_err(port->dev, "DMA slave single fail\n");
> > return;
> > }
>
> The serial driver is for the sirf platform, which uses the sirf-dma
> dmaengine driver, and that particular driver has an incorrect
> dma_prep_cyclic implementation, so I think we also need this fix:
>
> diff --git a/drivers/dma/sirf-dma.c b/drivers/dma/sirf-dma.c
> index 8f62edad51be..220c611c89ae 100644
> --- a/drivers/dma/sirf-dma.c
> +++ b/drivers/dma/sirf-dma.c
> @@ -775,7 +775,7 @@ sirfsoc_dma_prep_cyclic(struct dma_chan *chan, dma_addr_t
> addr,
>* BUFB
>*/
> if (buf_len != 2 * period_len)
> - return ERR_PTR(-EINVAL);
> + return NULL;
>
> /* Get free descriptor */
> spin_lock_irqsave(&schan->lock, iflags);
>
>
> Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Re: [PATCH] serial: sirf: Simplify a test
On Tuesday, November 1, 2016 8:03:33 AM CET Christophe JAILLET wrote:
> 'dmaengine_prep_dma_cyclic()' does not return an error pointer, so the test
> can be simplified to be more consistent.
>
> Signed-off-by: Christophe JAILLET
The change looks correct in principle. It would be good to automate looking
for other instances of this bug. How did you find it? Do you have e.g. a
coccinelle script or did you just stumble over the issue by accident?
There is one problem with your patch:
> drivers/tty/serial/sirfsoc_uart.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/sirfsoc_uart.c
> b/drivers/tty/serial/sirfsoc_uart.c
> index b186c9c4f850..666ca3156961 100644
> --- a/drivers/tty/serial/sirfsoc_uart.c
> +++ b/drivers/tty/serial/sirfsoc_uart.c
> @@ -609,7 +609,7 @@ static void sirfsoc_uart_start_next_rx_dma(struct
> uart_port *port)
> sirfport->rx_dma_items.dma_addr, SIRFSOC_RX_DMA_BUF_SIZE,
> SIRFSOC_RX_DMA_BUF_SIZE / 2,
> DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT);
> - if (IS_ERR_OR_NULL(sirfport->rx_dma_items.desc)) {
> + if (!sirfport->rx_dma_items.desc) {
> dev_err(port->dev, "DMA slave single fail\n");
> return;
> }
The serial driver is for the sirf platform, which uses the sirf-dma
dmaengine driver, and that particular driver has an incorrect
dma_prep_cyclic implementation, so I think we also need this fix:
diff --git a/drivers/dma/sirf-dma.c b/drivers/dma/sirf-dma.c
index 8f62edad51be..220c611c89ae 100644
--- a/drivers/dma/sirf-dma.c
+++ b/drivers/dma/sirf-dma.c
@@ -775,7 +775,7 @@ sirfsoc_dma_prep_cyclic(struct dma_chan *chan, dma_addr_t
addr,
* BUFB
*/
if (buf_len != 2 * period_len)
- return ERR_PTR(-EINVAL);
+ return NULL;
/* Get free descriptor */
spin_lock_irqsave(&schan->lock, iflags);
Arnd
[PATCH] serial: sirf: Simplify a test
'dmaengine_prep_dma_cyclic()' does not return an error pointer, so the test
can be simplified to be more consistent.
Signed-off-by: Christophe JAILLET
---
drivers/tty/serial/sirfsoc_uart.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/sirfsoc_uart.c
b/drivers/tty/serial/sirfsoc_uart.c
index b186c9c4f850..666ca3156961 100644
--- a/drivers/tty/serial/sirfsoc_uart.c
+++ b/drivers/tty/serial/sirfsoc_uart.c
@@ -609,7 +609,7 @@ static void sirfsoc_uart_start_next_rx_dma(struct uart_port
*port)
sirfport->rx_dma_items.dma_addr, SIRFSOC_RX_DMA_BUF_SIZE,
SIRFSOC_RX_DMA_BUF_SIZE / 2,
DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT);
- if (IS_ERR_OR_NULL(sirfport->rx_dma_items.desc)) {
+ if (!sirfport->rx_dma_items.desc) {
dev_err(port->dev, "DMA slave single fail\n");
return;
}
--
2.9.3
