* Re: [PATCH] media: meson: Add NULL check after the call to kmalloc()
From: Greg KH @ 2019-09-04 8:45 UTC (permalink / raw)
To: Austin Kim
Cc: mjourdan, devel, khilman, linux-kernel, linux-amlogic, mchehab,
linux-arm-kernel, linux-media
In-Reply-To: <20190904082232.GA171180@LGEARND20B15>
On Wed, Sep 04, 2019 at 05:22:32PM +0900, Austin Kim wrote:
> If the kmalloc() return NULL, the NULL pointer dereference will occur.
> new_ts->ts = ts;
>
> Add exception check after the call to kmalloc() is made.
>
> Signed-off-by: Austin Kim <austindh.kim@gmail.com>
> ---
> drivers/staging/media/meson/vdec/vdec_helpers.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
> index f16948b..e7e56d5 100644
> --- a/drivers/staging/media/meson/vdec/vdec_helpers.c
> +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
> @@ -206,6 +206,10 @@ void amvdec_add_ts_reorder(struct amvdec_session *sess, u64 ts, u32 offset)
> unsigned long flags;
>
> new_ts = kmalloc(sizeof(*new_ts), GFP_KERNEL);
> + if (!new_ts) {
> + dev_err(sess->core->dev, "Failed to kmalloc()\n");
Did you run this through checkpatch? I think it will say that this line
is not ok.
> + return;
Shouldn't you return an -ENOMEM error somehow?
thanks,
greg k-h
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] media: meson: Add NULL check after the call to kmalloc()
From: Rasmus Villemoes @ 2019-09-04 8:43 UTC (permalink / raw)
To: Austin Kim, mchehab, khilman
Cc: mjourdan, devel, gregkh, linux-kernel, linux-amlogic,
linux-arm-kernel, linux-media
In-Reply-To: <20190904082232.GA171180@LGEARND20B15>
On 04/09/2019 10.22, Austin Kim wrote:
> If the kmalloc() return NULL, the NULL pointer dereference will occur.
> new_ts->ts = ts;
>
> Add exception check after the call to kmalloc() is made.
>
> Signed-off-by: Austin Kim <austindh.kim@gmail.com>
> ---
> drivers/staging/media/meson/vdec/vdec_helpers.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
> index f16948b..e7e56d5 100644
> --- a/drivers/staging/media/meson/vdec/vdec_helpers.c
> +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
> @@ -206,6 +206,10 @@ void amvdec_add_ts_reorder(struct amvdec_session *sess, u64 ts, u32 offset)
> unsigned long flags;
>
> new_ts = kmalloc(sizeof(*new_ts), GFP_KERNEL);
> + if (!new_ts) {
> + dev_err(sess->core->dev, "Failed to kmalloc()\n");
> + return;
> + }
> new_ts->ts = ts;
> new_ts->offset = offset;
No need to printk() on error, AFAIK the mm subsystem should already make
some noise if an allocation fails.
This is not a proper fix - you need to make the function return an error
(-ENOMEM) to let the caller know allocation failed, and allow that to
propagate the error. There's only one caller, which already seems
capable of returning errors (there's an -EAGAIN), so it shouldn't be
that hard - though of course one needs to undo what has been done so far.
Also, unrelated to the kmalloc() handling: amvdec_add_ts_reorder() could
be moved to esparser.c and made static, or at the very least the
EXPORT_SYMBOL can be removed since vdec_helpers.o is linked in to the
same module as the sole user. That probably goes for all the
EXPORT_SYMBOL(amvdec_*).
Rasmus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [GIT PULL 1/3] soc: samsung: Exynos for v5.4
From: Krzysztof Kozlowski @ 2019-09-04 8:37 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
SoC Team, arm-soc, Kukjin Kim, Olof Johansson, Linux ARM
In-Reply-To: <CAK8P3a1_Qw=OB31yOCrpPs8Ys+=9tt4Pnyd=3+2JGzRXJV1KAw@mail.gmail.com>
On Tue, 3 Sep 2019 at 19:21, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Aug 22, 2019 at 8:35 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On Wed, Aug 21, 2019 at 09:51:09AM +0200, Krzysztof Kozlowski wrote:
> > > On Fri, 16 Aug 2019 at 18:30, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > >
> > > > The following changes since commit 5f9e832c137075045d15cd6899ab0505cfb2ca4b:
> > > >
> > > > Linus 5.3-rc1 (2019-07-21 14:05:38 -0700)
> > > >
> > > > are available in the Git repository at:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux.git tags/samsung-drivers-5.4
> > > >
> > > > for you to fetch changes up to 40d8aff614f71ab3cab20785b4f213e3802d4e87:
> > > >
> > > > soc: samsung: chipid: Convert exynos-chipid driver to use the regmap API (2019-08-15 20:25:25 +0200)
> > > >
> > > > ----------------------------------------------------------------
> > > > Samsung soc drivers changes for v5.4
> > > >
> > > > Add Exynos Chipid driver for identification of product IDs and SoC
> > > > revisions. The driver also exposes chipid regmap, later to be used by
> > > > Exynos Adaptive Supply Voltage driver (adjusting voltages to different
> > > > revisions of same SoC).
> > >
> > > It turns out that it brings troubles (code is executed on every
> > > platform polluting logs because it is an initcall, not a driver) so
> > > Sylwester (submitter) asked to skip the submission.
> > >
> > > Please ignore the pull request.
> >
> > I talked with Sylwester and Bartlomiej who contributed the chipid driver
> > and they provided small incremental fixes. The driver is still useful
> > and in the future it will be expanded towards AVS. Therefore please pull
> > it or optionally wait a week and I will send incremental pull request
> > with fixes.
>
> Pulled into arm/drivers for now.
>
> I have drafted a related patch recently, regarding the related
> arch/arm/plat-samsung/cpu.c file. This is part of a longer series
> I'm working on, see https://pastebin.com/ZqeU3Mth for the
> current version of this patch.
You can then also adjust the include path in arch/arm/mach-exynos/Makefile.
> The observation is that mach-exynos
> is almost completely independent of plat-samsung these days, and my
> patch removes the last obstacle from separating the two. I have
> another set of patches to do the same for mach-s5pv210 (which shares
> half of its pm.c with plat-samsung, but nothing else).
Great!
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH -next] ASoC: sirf-audio: use devm_platform_ioremap_resource() to simplify code
From: YueHaibing @ 2019-09-04 8:34 UTC (permalink / raw)
To: lgirdwood, broonie, perex, tiwai, baohua, gregkh, kstewart, tglx,
allison, yuehaibing, pakki001
Cc: alsa-devel, linux-kernel, linux-arm-kernel
Use devm_platform_ioremap_resource() to simplify the code a bit.
This is detected by coccinelle.
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
sound/soc/codecs/sirf-audio-codec.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/sound/soc/codecs/sirf-audio-codec.c b/sound/soc/codecs/sirf-audio-codec.c
index 9009a74..a061d78 100644
--- a/sound/soc/codecs/sirf-audio-codec.c
+++ b/sound/soc/codecs/sirf-audio-codec.c
@@ -459,7 +459,6 @@ static int sirf_audio_codec_driver_probe(struct platform_device *pdev)
int ret;
struct sirf_audio_codec *sirf_audio_codec;
void __iomem *base;
- struct resource *mem_res;
sirf_audio_codec = devm_kzalloc(&pdev->dev,
sizeof(struct sirf_audio_codec), GFP_KERNEL);
@@ -468,8 +467,7 @@ static int sirf_audio_codec_driver_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, sirf_audio_codec);
- mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- base = devm_ioremap_resource(&pdev->dev, mem_res);
+ base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(base))
return PTR_ERR(base);
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [RFC PATCH V2 4/4] platform: mtk-isp: Add Mediatek FD driver
From: Tomasz Figa @ 2019-09-04 8:25 UTC (permalink / raw)
To: Jerry-ch Chen
Cc: devicetree@vger.kernel.org, Sean Cheng (鄭昇弘),
laurent.pinchart+renesas@ideasonboard.com,
Rynn Wu (吳育恩), srv_heupstream,
Po-Yang Huang (黃柏陽), mchehab@kernel.org,
suleiman@chromium.org, shik@chromium.org,
Jungo Lin (林明俊),
Sj Huang (黃信璋), yuzhao@chromium.org,
linux-mediatek@lists.infradead.org, zwisler@chromium.org,
matthias.bgg@gmail.com, Christie Yu (游雅惠),
Frederic Chen (陳俊元), hans.verkuil@cisco.com,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <1567584577.22453.11.camel@mtksdccf07>
On Wed, Sep 4, 2019 at 5:09 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
>
> Hi Tomasz,
>
> On Wed, 2019-09-04 at 14:34 +0800, Tomasz Figa wrote:
> > On Wed, Sep 4, 2019 at 3:09 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > >
> > > Hi Tomasz,
> > >
> > > On Wed, 2019-09-04 at 12:15 +0800, Tomasz Figa wrote:
> > > > On Wed, Sep 4, 2019 at 12:38 PM Jerry-ch Chen
> > > > <Jerry-ch.Chen@mediatek.com> wrote:
> > > > >
> > > > > Hi Tomasz,
> > > > >
> > > > > On Tue, 2019-09-03 at 20:05 +0800, Tomasz Figa wrote:
> > > > > > On Tue, Sep 3, 2019 at 8:46 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > >
> > > > > > > Hi Tomasz,
> > > > > > >
> > > > > > > On Tue, 2019-09-03 at 15:04 +0800, Tomasz Figa wrote:
> > > > > > > > On Tue, Sep 3, 2019 at 3:44 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, 2019-09-03 at 13:19 +0800, Tomasz Figa wrote:
> > > > > > > > > > On Mon, Sep 2, 2019 at 8:47 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Tomasz,
> > > > > > > > > > >
> > > > > > > > > > > On Fri, 2019-08-30 at 16:33 +0800, Tomasz Figa wrote:
> > > > > > > > > > > > On Wed, Aug 28, 2019 at 11:00 AM Jerry-ch Chen
> > > > > > > > > > > > <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hi Tomasz,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Mon, 2019-08-26 at 14:36 +0800, Tomasz Figa wrote:
> > > > > > > > > > > > > > Hi Jerry,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Sun, Aug 25, 2019 at 6:18 PM Jerry-ch Chen
> > > > > > > > > > > > > > <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Hi Tomasz,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > On Fri, 2019-08-02 at 16:28 +0800, Tomasz Figa wrote:
> > > > > > > > > > > > > > > > Hi Jerry,
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > On Tue, Jul 09, 2019 at 04:41:12PM +0800, Jerry-ch Chen wrote:
> > > > > [snip]
> > > > > > > > > > > > > > [snip]
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > +static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq)
> > > > > > > > > > > > > > > > > +{
> > > > > > > > > > > > > > > > > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > > > > > > > > > > > > > > > > + struct vb2_buffer *vb;
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > How do we guarantee here that the hardware isn't still accessing the buffers
> > > > > > > > > > > > > > > > removed below?
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Maybe we can check the driver state flag and aborting the unfinished
> > > > > > > > > > > > > > > jobs?
> > > > > > > > > > > > > > > (fd_hw->state == FD_ENQ)
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Yes, we need to either cancel or wait for the currently processing
> > > > > > > > > > > > > > job. It depends on hardware capabilities, but cancelling is generally
> > > > > > > > > > > > > > preferred for the lower latency.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > Ok, it the state is ENQ, then we can disable the FD hw by controlling
> > > > > > > > > > > > > the registers.
> > > > > > > > > > > > >
> > > > > > > > > > > > > for example:
> > > > > > > > > > > > > writel(0x0, fd->fd_base + FD_HW_ENABLE);
> > > > > > > > > > > > > writel(0x0, fd->fd_base + FD_INT_EN);
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > What's exactly the effect of writing 0 to FD_HW_ENABLE?
> > > > > > > > > > > >
> > > > > > > > > > > Sorry, my last reply didn't solve the question,
> > > > > > > > > > > we should implement a mtk_fd_job_abort() for v4l2_m2m_ops().
> > > > > > > > > > >
> > > > > > > > > > > which is able to readl_poll_timeout_atomic()
> > > > > > > > > > > and check the HW busy bits in the register FD_INT_EN;
> > > > > > > > > > >
> > > > > > > > > > > if they are not cleared until timeout, we could handle the last
> > > > > > > > > > > processing job.
> > > > > > > > > > > Otherwise, the FD irq handler should have handled the last processing
> > > > > > > > > > > job and we could continue the stop_streaming().
> > > > > > > > > > >
> > > > > > > > > > > For job_abort():
> > > > > > > > > > > static void mtk_fd_job_abort(void *priv)
> > > > > > > > > > > {
> > > > > > > > > > > struct mtk_fd_ctx *ctx = priv;
> > > > > > > > > > > struct mtk_fd_dev *fd = ctx->fd_dev;
> > > > > > > > > > > u32 val;
> > > > > > > > > > > u32 ret;
> > > > > > > > > > >
> > > > > > > > > > > ret = readl_poll_timeout_atomic(fd->fd_base + MTK_FD_REG_OFFSET_INT_EN,
> > > > > > > > > > > val,
> > > > > > > > > > > (val & MTK_FD_HW_BUSY_MASK) ==
> > > > > > > > > > > MTK_FD_HW_STATE_IS_BUSY,
> > > > > > > > > > > USEC_PER_MSEC, MTK_FD_STOP_HW_TIMEOUT);
> > > > > > > > > >
> > > > > > > > > > Hmm, would it be possible to avoid the busy wait by having a
> > > > > > > > > > completion that could be signalled from the interrupt handler?
> > > > > > > > > >
> > > > > > > > > > Best regards,
> > > > > > > > > > Tomasz
> > > > > > > > >
> > > > > > > > > I suppose that would be wakeup a wait queue in the interrupt handler,
> > > > > > > > > the the wait_event_interrupt_timeout() will be used in here and system
> > > > > > > > > suspend e.g. mtk_fd_suspend().
> > > > > > > >
> > > > > > > > Yes, that should work.
> > > > > > > >
> > > > > > > > > Or do you suggest to wait_event_interrupt_timeout() every frame in the
> > > > > > > > > mtk_fd_ipi_handler()?
> > > > > > > >
> > > > > > > > Nope, we shouldn't need that.
> > > > > > > >
> > > > > > > > > I think maybe the readl_poll_timeout_atomic would be good enough.
> > > > > > > > >
> > > > > > > >
> > > > > > > > Not really. Busy waiting should be avoided as much as possible. What's
> > > > > > > > the point of entering suspend if you end up burning the power by
> > > > > > > > spinning the CPU for some milliseconds?
> > > > > > > >
> > > > > > > Ok, I see, busy waiting is not a good idea, I will use the wait queue
> > > > > > > instead. the job abort will refine as following:
> > > > > > >
> > > > > > > static void mtk_fd_job_abort(void *priv)
> > > > > > > {
> > > > > > > struct mtk_fd_ctx *ctx = priv;
> > > > > > > struct mtk_fd_dev *fd = ctx->fd_dev;
> > > > > > > u32 ret;
> > > > > > >
> > > > > > > ret = wait_event_interruptible_timeout
> > > > > > > (fd->wq, (fd->fd_irq_result & MTK_FD_HW_IRQ_MASK),
> > > > > > > usecs_to_jiffies(50000));
> > > > > > > if (ret)
> > > > > > > mtk_fd_hw_job_finish(fd, VB2_BUF_STATE_ERROR);
> > > > > > > dev_dbg(fd->dev, "%s, ret:%d\n", __func__, ret);
> > > > > > >
> > > > > > > fd->fd_irq_result = 0;
> > > > > > > }
> > > > > > >
> > > > > > > static struct v4l2_m2m_ops fd_m2m_ops = {
> > > > > > > .device_run = mtk_fd_device_run,
> > > > > > > .job_abort = mtk_fd_job_abort,
> > > > > >
> > > > > > I'm not sure we should be using the functon above as the .job_abort
> > > > > > callback. It's expected to abort the job, but we're just waiting for
> > > > > > it to finish. I think we should just call mtk_fd_job_abort() manually
> > > > > > from .stop_streaming.
> > > > > >
> > > > >
> > > > > Ok, I will fix it.
> > > > >
> > > > > > > };
> > > > > > >
> > > > > > > and we could use it in suspend.
> > > > > > > static int mtk_fd_suspend(struct device *dev)
> > > > > > > {
> > > > > > > struct mtk_fd_dev *fd = dev_get_drvdata(dev);
> > > > > > >
> > > > > > > if (pm_runtime_suspended(dev))
> > > > > > > return 0;
> > > > > > >
> > > > > > > if (fd->fd_stream_count)
> > > > > > > mtk_fd_job_abort(fd->ctx);
> > > > > > >
> > > > > > > /* suspend FD HW */
> > > > > > > writel(0x0, fd->fd_base + MTK_FD_REG_OFFSET_INT_EN);
> > > > > > > writel(0x0, fd->fd_base + MTK_FD_REG_OFFSET_HW_ENABLE);
> > > > > > > clk_disable_unprepare(fd->fd_clk);
> > > > > > > dev_dbg(dev, "%s:disable clock\n", __func__);
> > > > > > >
> > > > > > > return 0;
> > > > > > > }
> > > > > > >
> > > > > > > static irqreturn_t mtk_fd_irq(int irq, void *data)
> > > > > > > {
> > > > > > > struct mtk_fd_dev *fd = (struct mtk_fd_dev *)data;
> > > > > > >
> > > > > > > fd->fd_irq_result = readl(fd->fd_base + MTK_FD_REG_OFFSET_INT_VAL);
> > > > > > > wake_up_interruptible(&fd->wq);
> > > > > >
> > > > > > The wake up should be done at the very end of this function. Otherwise
> > > > > > we end up with m2m framework racing with the mtk_fd_hw_job_finish()
> > > > > > below.
> > > > > >
> > > > > > Also, I'd use a completion here rather than an open coded wait and
> > > > > > wake-up. The driver could reinit_completion() before queuing a job to
> > > > > > the hardware and the IRQ handler would complete(). There would be no
> > > > > > need to store the IRQ flags in driver data anymore.
> > > > > Ok, It will be refined as following:
> > > > >
> > > > > suspend and stop streaming will call mtk_fd_job_abort()
> > > > >
> > > > > static void mtk_fd_job_abort(struct mtk_fd_dev *fd)
> > > > > {
> > > > > u32 ret;
> > > > >
> > > > > ret = wait_for_completion_timeout(&fd->fd_irq_done,
> > > > > msecs_to_jiffies(MTK_FD_HW_TIMEOUT));
> > > > > if (ret)
> > > >
> > > > For the _timeout variants, !ret means the timeout and ret > 0 means
> > > > that the wait ended successfully before.
> > > >
> > > Thanks, fixed.
> > >
> > > > Also please make sure that the timeout is big enough not to happen
> > > > normally on a heavily loaded system. Something like 1 second should be
> > > > good.
> > > >
> > > Ok, I will make it 1 second
> > > #define MTK_FD_HW_TIMEOUT 1000
> > >
> > > Also, I will add a condition in mtk_fd_vb2_stop_streaming(), since it
> > > would be called twice, now it works fine whether I reuse the condition
> > > with mtk_fd_hw_disconnect or not, however, I think do it before buffer
> > > remove looks more reasonable.
> > >
> > > static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq)
> > > {
> > > struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > > struct mtk_fd_dev *fd = ctx->fd_dev;
> > > struct vb2_v4l2_buffer *vb;
> > > struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> > > struct v4l2_m2m_queue_ctx *queue_ctx;
> > >
> > > if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> > > mtk_fd_job_abort(fd);
> >
> > We need to do it regardless of the queue type, otherwise we could
> > still free CAPTURE buffers before the hardware releases them.
> >
>
> I think we may read the fd->fd_irq_done.done and do wait for completion
> if it's not being done.
> How do you think?
>
> static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq)
> {
> struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> struct mtk_fd_dev *fd = ctx->fd_dev;
> struct vb2_v4l2_buffer *vb;
> struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> struct v4l2_m2m_queue_ctx *queue_ctx;
> u32 ret;
>
> if (!fd->fd_irq_done.done)
We shouldn't access internal fields of completion.
> ret = wait_for_completion_timeout(&fd->fd_irq_done,
> msecs_to_jiffies(
> MTK_FD_HW_TIMEOUT));
> queue_ctx = V4L2_TYPE_IS_OUTPUT(vq->type) ?
> &m2m_ctx->out_q_ctx :
> &m2m_ctx->cap_q_ctx;
> while ((vb = v4l2_m2m_buf_remove(queue_ctx)))
> v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR);
>
> if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> mtk_fd_hw_disconnect(fd);
> }
>
> I've also tried to wait completion unconditionally for both queues and
> the second time will wait until timeout, as a result, it takes longer to
> swap the camera every time and close the camera app.
I think it should work better if we call complete_all() instead of complete().
Best regards,
Tomasz
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [GIT PULL 2/3] ARM: samsung: mach for v5.4
From: Krzysztof Kozlowski @ 2019-09-04 8:30 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
SoC Team, arm-soc, Kukjin Kim, Olof Johansson, Linux ARM
In-Reply-To: <CAK8P3a0sa8WgcNnL8jusYKFr22FqGnut4kRKM-1QcB8G+ygnMg@mail.gmail.com>
On Tue, 3 Sep 2019 at 19:32, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Aug 21, 2019 at 9:52 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On Fri, 16 Aug 2019 at 18:30, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> > > ----------------------------------------------------------------
> > > Linus Walleij (1):
> > > ARM: samsung: Include GPIO driver header
> > >
> > > Pankaj Dubey (1):
> > > ARM: exynos: Enable exynos-chipid driver
> >
> > This last patch should be dropped so I will rework the pull request
> > and send later v2. Please ignore it for now.
>
> I don't see the v2 yet. Are you still planning to send it?
Yes, I have two more patches so I plan incremental pull today or tomorrow.
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] media: meson: Add NULL check after the call to kmalloc()
From: Austin Kim @ 2019-09-04 8:22 UTC (permalink / raw)
To: mchehab, khilman
Cc: mjourdan, devel, gregkh, linux-kernel, linux-amlogic,
linux-arm-kernel, linux-media
If the kmalloc() return NULL, the NULL pointer dereference will occur.
new_ts->ts = ts;
Add exception check after the call to kmalloc() is made.
Signed-off-by: Austin Kim <austindh.kim@gmail.com>
---
drivers/staging/media/meson/vdec/vdec_helpers.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
index f16948b..e7e56d5 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.c
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
@@ -206,6 +206,10 @@ void amvdec_add_ts_reorder(struct amvdec_session *sess, u64 ts, u32 offset)
unsigned long flags;
new_ts = kmalloc(sizeof(*new_ts), GFP_KERNEL);
+ if (!new_ts) {
+ dev_err(sess->core->dev, "Failed to kmalloc()\n");
+ return;
+ }
new_ts->ts = ts;
new_ts->offset = offset;
--
2.6.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH V7 1/3] mm/hotplug: Reorder memblock_[free|remove]() calls in try_remove_memory()
From: David Hildenbrand @ 2019-09-04 8:16 UTC (permalink / raw)
To: Anshuman Khandual, linux-mm, linux-kernel, linux-arm-kernel, akpm,
catalin.marinas, will
Cc: mark.rutland, mhocko, steve.capper, ira.weiny, suzuki.poulose,
mgorman, steven.price, broonie, cai, ard.biesheuvel, cpandya,
arunks, dan.j.williams, Robin.Murphy, logang, valentin.schneider,
osalvador
In-Reply-To: <1567503958-25831-2-git-send-email-anshuman.khandual@arm.com>
On 03.09.19 11:45, Anshuman Khandual wrote:
> Memory hot remove uses get_nid_for_pfn() while tearing down linked sysfs
> entries between memory block and node. It first checks pfn validity with
> pfn_valid_within() before fetching nid. With CONFIG_HOLES_IN_ZONE config
> (arm64 has this enabled) pfn_valid_within() calls pfn_valid().
>
> pfn_valid() is an arch implementation on arm64 (CONFIG_HAVE_ARCH_PFN_VALID)
> which scans all mapped memblock regions with memblock_is_map_memory(). This
> creates a problem in memory hot remove path which has already removed given
> memory range from memory block with memblock_[remove|free] before arriving
> at unregister_mem_sect_under_nodes(). Hence get_nid_for_pfn() returns -1
> skipping subsequent sysfs_remove_link() calls leaving node <-> memory block
> sysfs entries as is. Subsequent memory add operation hits BUG_ON() because
> of existing sysfs entries.
Since
commit 60bb462fc7adb06ebee3beb5a4af6c7e6182e248
Author: David Hildenbrand <david@redhat.com>
Date: Wed Aug 28 13:57:15 2019 +1000
drivers/base/node.c: simplify unregister_memory_block_under_nodes()
that problem should be gone. There is no get_nid_for_pfn() call anymore.
So this patch should no longer be necessary - but as I said during
earlier versions of this patch, the re-ordering might still make sense
for consistency (removing stuff in the reverse order they were added).
You'll have to rephrase the description then.
>
> [ 62.007176] NUMA: Unknown node for memory at 0x680000000, assuming node 0
> [ 62.052517] ------------[ cut here ]------------
> [ 62.053211] kernel BUG at mm/memory_hotplug.c:1143!
> [ 62.053868] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
> [ 62.054589] Modules linked in:
> [ 62.054999] CPU: 19 PID: 3275 Comm: bash Not tainted 5.1.0-rc2-00004-g28cea40b2683 #41
> [ 62.056274] Hardware name: linux,dummy-virt (DT)
> [ 62.057166] pstate: 40400005 (nZcv daif +PAN -UAO)
> [ 62.058083] pc : add_memory_resource+0x1cc/0x1d8
> [ 62.058961] lr : add_memory_resource+0x10c/0x1d8
> [ 62.059842] sp : ffff0000168b3ce0
> [ 62.060477] x29: ffff0000168b3ce0 x28: ffff8005db546c00
> [ 62.061501] x27: 0000000000000000 x26: 0000000000000000
> [ 62.062509] x25: ffff0000111ef000 x24: ffff0000111ef5d0
> [ 62.063520] x23: 0000000000000000 x22: 00000006bfffffff
> [ 62.064540] x21: 00000000ffffffef x20: 00000000006c0000
> [ 62.065558] x19: 0000000000680000 x18: 0000000000000024
> [ 62.066566] x17: 0000000000000000 x16: 0000000000000000
> [ 62.067579] x15: ffffffffffffffff x14: ffff8005e412e890
> [ 62.068588] x13: ffff8005d6b105d8 x12: 0000000000000000
> [ 62.069610] x11: ffff8005d6b10490 x10: 0000000000000040
> [ 62.070615] x9 : ffff8005e412e898 x8 : ffff8005e412e890
> [ 62.071631] x7 : ffff8005d6b105d8 x6 : ffff8005db546c00
> [ 62.072640] x5 : 0000000000000001 x4 : 0000000000000002
> [ 62.073654] x3 : ffff8005d7049480 x2 : 0000000000000002
> [ 62.074666] x1 : 0000000000000003 x0 : 00000000ffffffef
> [ 62.075685] Process bash (pid: 3275, stack limit = 0x00000000d754280f)
> [ 62.076930] Call trace:
> [ 62.077411] add_memory_resource+0x1cc/0x1d8
> [ 62.078227] __add_memory+0x70/0xa8
> [ 62.078901] probe_store+0xa4/0xc8
> [ 62.079561] dev_attr_store+0x18/0x28
> [ 62.080270] sysfs_kf_write+0x40/0x58
> [ 62.080992] kernfs_fop_write+0xcc/0x1d8
> [ 62.081744] __vfs_write+0x18/0x40
> [ 62.082400] vfs_write+0xa4/0x1b0
> [ 62.083037] ksys_write+0x5c/0xc0
> [ 62.083681] __arm64_sys_write+0x18/0x20
> [ 62.084432] el0_svc_handler+0x88/0x100
> [ 62.085177] el0_svc+0x8/0xc
>
> Re-ordering memblock_[free|remove]() with arch_remove_memory() solves the
> problem on arm64 as pfn_valid() behaves correctly and returns positive
> as memblock for the address range still exists. arch_remove_memory()
> removes applicable memory sections from zone with __remove_pages() and
> tears down kernel linear mapping. Removing memblock regions afterwards
> is safe because there is no other memblock (bootmem) allocator user that
> late. So nobody is going to allocate from the removed range just to blow
> up later. Also nobody should be using the bootmem allocated range else
> we wouldn't allow to remove it. So reordering is indeed safe.
>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Oscar Salvador <osalvador@suse.de>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> Acked-by: Michal Hocko <mhocko@suse.com>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> mm/memory_hotplug.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index c73f09913165..355c466e0621 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1770,13 +1770,13 @@ static int __ref try_remove_memory(int nid, u64 start, u64 size)
>
> /* remove memmap entry */
> firmware_map_remove(start, start + size, "System RAM");
> - memblock_free(start, size);
> - memblock_remove(start, size);
>
> /* remove memory block devices before removing memory */
> remove_memory_block_devices(start, size);
>
> arch_remove_memory(nid, start, size, NULL);
> + memblock_free(start, size);
> + memblock_remove(start, size);
> __release_memory_resource(start, size);
>
> try_offline_node(nid);
>
--
Thanks,
David / dhildenb
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC PATCH V2 4/4] platform: mtk-isp: Add Mediatek FD driver
From: Jerry-ch Chen @ 2019-09-04 8:09 UTC (permalink / raw)
To: Tomasz Figa
Cc: devicetree@vger.kernel.org, Sean Cheng (鄭昇弘),
laurent.pinchart+renesas@ideasonboard.com,
Rynn Wu (吳育恩), srv_heupstream,
Po-Yang Huang (黃柏陽), mchehab@kernel.org,
suleiman@chromium.org, shik@chromium.org,
Jungo Lin (林明俊),
Sj Huang (黃信璋), yuzhao@chromium.org,
linux-mediatek@lists.infradead.org, zwisler@chromium.org,
matthias.bgg@gmail.com, Christie Yu (游雅惠),
Frederic Chen (陳俊元), hans.verkuil@cisco.com,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <CAAFQd5AxTQPD+nP9CJs45QTzGHKssjv3vRtMqHONABfp12afYw@mail.gmail.com>
Hi Tomasz,
On Wed, 2019-09-04 at 14:34 +0800, Tomasz Figa wrote:
> On Wed, Sep 4, 2019 at 3:09 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> >
> > Hi Tomasz,
> >
> > On Wed, 2019-09-04 at 12:15 +0800, Tomasz Figa wrote:
> > > On Wed, Sep 4, 2019 at 12:38 PM Jerry-ch Chen
> > > <Jerry-ch.Chen@mediatek.com> wrote:
> > > >
> > > > Hi Tomasz,
> > > >
> > > > On Tue, 2019-09-03 at 20:05 +0800, Tomasz Figa wrote:
> > > > > On Tue, Sep 3, 2019 at 8:46 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > >
> > > > > > Hi Tomasz,
> > > > > >
> > > > > > On Tue, 2019-09-03 at 15:04 +0800, Tomasz Figa wrote:
> > > > > > > On Tue, Sep 3, 2019 at 3:44 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > >
> > > > > > > > On Tue, 2019-09-03 at 13:19 +0800, Tomasz Figa wrote:
> > > > > > > > > On Mon, Sep 2, 2019 at 8:47 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Tomasz,
> > > > > > > > > >
> > > > > > > > > > On Fri, 2019-08-30 at 16:33 +0800, Tomasz Figa wrote:
> > > > > > > > > > > On Wed, Aug 28, 2019 at 11:00 AM Jerry-ch Chen
> > > > > > > > > > > <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Hi Tomasz,
> > > > > > > > > > > >
> > > > > > > > > > > > On Mon, 2019-08-26 at 14:36 +0800, Tomasz Figa wrote:
> > > > > > > > > > > > > Hi Jerry,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Sun, Aug 25, 2019 at 6:18 PM Jerry-ch Chen
> > > > > > > > > > > > > <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Hi Tomasz,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Fri, 2019-08-02 at 16:28 +0800, Tomasz Figa wrote:
> > > > > > > > > > > > > > > Hi Jerry,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > On Tue, Jul 09, 2019 at 04:41:12PM +0800, Jerry-ch Chen wrote:
> > > > [snip]
> > > > > > > > > > > > > [snip]
> > > > > > > > > > > > >
> > > > > > > > > > > > > > > > +static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq)
> > > > > > > > > > > > > > > > +{
> > > > > > > > > > > > > > > > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > > > > > > > > > > > > > > > + struct vb2_buffer *vb;
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > How do we guarantee here that the hardware isn't still accessing the buffers
> > > > > > > > > > > > > > > removed below?
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > Maybe we can check the driver state flag and aborting the unfinished
> > > > > > > > > > > > > > jobs?
> > > > > > > > > > > > > > (fd_hw->state == FD_ENQ)
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > Yes, we need to either cancel or wait for the currently processing
> > > > > > > > > > > > > job. It depends on hardware capabilities, but cancelling is generally
> > > > > > > > > > > > > preferred for the lower latency.
> > > > > > > > > > > > >
> > > > > > > > > > > > Ok, it the state is ENQ, then we can disable the FD hw by controlling
> > > > > > > > > > > > the registers.
> > > > > > > > > > > >
> > > > > > > > > > > > for example:
> > > > > > > > > > > > writel(0x0, fd->fd_base + FD_HW_ENABLE);
> > > > > > > > > > > > writel(0x0, fd->fd_base + FD_INT_EN);
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > What's exactly the effect of writing 0 to FD_HW_ENABLE?
> > > > > > > > > > >
> > > > > > > > > > Sorry, my last reply didn't solve the question,
> > > > > > > > > > we should implement a mtk_fd_job_abort() for v4l2_m2m_ops().
> > > > > > > > > >
> > > > > > > > > > which is able to readl_poll_timeout_atomic()
> > > > > > > > > > and check the HW busy bits in the register FD_INT_EN;
> > > > > > > > > >
> > > > > > > > > > if they are not cleared until timeout, we could handle the last
> > > > > > > > > > processing job.
> > > > > > > > > > Otherwise, the FD irq handler should have handled the last processing
> > > > > > > > > > job and we could continue the stop_streaming().
> > > > > > > > > >
> > > > > > > > > > For job_abort():
> > > > > > > > > > static void mtk_fd_job_abort(void *priv)
> > > > > > > > > > {
> > > > > > > > > > struct mtk_fd_ctx *ctx = priv;
> > > > > > > > > > struct mtk_fd_dev *fd = ctx->fd_dev;
> > > > > > > > > > u32 val;
> > > > > > > > > > u32 ret;
> > > > > > > > > >
> > > > > > > > > > ret = readl_poll_timeout_atomic(fd->fd_base + MTK_FD_REG_OFFSET_INT_EN,
> > > > > > > > > > val,
> > > > > > > > > > (val & MTK_FD_HW_BUSY_MASK) ==
> > > > > > > > > > MTK_FD_HW_STATE_IS_BUSY,
> > > > > > > > > > USEC_PER_MSEC, MTK_FD_STOP_HW_TIMEOUT);
> > > > > > > > >
> > > > > > > > > Hmm, would it be possible to avoid the busy wait by having a
> > > > > > > > > completion that could be signalled from the interrupt handler?
> > > > > > > > >
> > > > > > > > > Best regards,
> > > > > > > > > Tomasz
> > > > > > > >
> > > > > > > > I suppose that would be wakeup a wait queue in the interrupt handler,
> > > > > > > > the the wait_event_interrupt_timeout() will be used in here and system
> > > > > > > > suspend e.g. mtk_fd_suspend().
> > > > > > >
> > > > > > > Yes, that should work.
> > > > > > >
> > > > > > > > Or do you suggest to wait_event_interrupt_timeout() every frame in the
> > > > > > > > mtk_fd_ipi_handler()?
> > > > > > >
> > > > > > > Nope, we shouldn't need that.
> > > > > > >
> > > > > > > > I think maybe the readl_poll_timeout_atomic would be good enough.
> > > > > > > >
> > > > > > >
> > > > > > > Not really. Busy waiting should be avoided as much as possible. What's
> > > > > > > the point of entering suspend if you end up burning the power by
> > > > > > > spinning the CPU for some milliseconds?
> > > > > > >
> > > > > > Ok, I see, busy waiting is not a good idea, I will use the wait queue
> > > > > > instead. the job abort will refine as following:
> > > > > >
> > > > > > static void mtk_fd_job_abort(void *priv)
> > > > > > {
> > > > > > struct mtk_fd_ctx *ctx = priv;
> > > > > > struct mtk_fd_dev *fd = ctx->fd_dev;
> > > > > > u32 ret;
> > > > > >
> > > > > > ret = wait_event_interruptible_timeout
> > > > > > (fd->wq, (fd->fd_irq_result & MTK_FD_HW_IRQ_MASK),
> > > > > > usecs_to_jiffies(50000));
> > > > > > if (ret)
> > > > > > mtk_fd_hw_job_finish(fd, VB2_BUF_STATE_ERROR);
> > > > > > dev_dbg(fd->dev, "%s, ret:%d\n", __func__, ret);
> > > > > >
> > > > > > fd->fd_irq_result = 0;
> > > > > > }
> > > > > >
> > > > > > static struct v4l2_m2m_ops fd_m2m_ops = {
> > > > > > .device_run = mtk_fd_device_run,
> > > > > > .job_abort = mtk_fd_job_abort,
> > > > >
> > > > > I'm not sure we should be using the functon above as the .job_abort
> > > > > callback. It's expected to abort the job, but we're just waiting for
> > > > > it to finish. I think we should just call mtk_fd_job_abort() manually
> > > > > from .stop_streaming.
> > > > >
> > > >
> > > > Ok, I will fix it.
> > > >
> > > > > > };
> > > > > >
> > > > > > and we could use it in suspend.
> > > > > > static int mtk_fd_suspend(struct device *dev)
> > > > > > {
> > > > > > struct mtk_fd_dev *fd = dev_get_drvdata(dev);
> > > > > >
> > > > > > if (pm_runtime_suspended(dev))
> > > > > > return 0;
> > > > > >
> > > > > > if (fd->fd_stream_count)
> > > > > > mtk_fd_job_abort(fd->ctx);
> > > > > >
> > > > > > /* suspend FD HW */
> > > > > > writel(0x0, fd->fd_base + MTK_FD_REG_OFFSET_INT_EN);
> > > > > > writel(0x0, fd->fd_base + MTK_FD_REG_OFFSET_HW_ENABLE);
> > > > > > clk_disable_unprepare(fd->fd_clk);
> > > > > > dev_dbg(dev, "%s:disable clock\n", __func__);
> > > > > >
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > static irqreturn_t mtk_fd_irq(int irq, void *data)
> > > > > > {
> > > > > > struct mtk_fd_dev *fd = (struct mtk_fd_dev *)data;
> > > > > >
> > > > > > fd->fd_irq_result = readl(fd->fd_base + MTK_FD_REG_OFFSET_INT_VAL);
> > > > > > wake_up_interruptible(&fd->wq);
> > > > >
> > > > > The wake up should be done at the very end of this function. Otherwise
> > > > > we end up with m2m framework racing with the mtk_fd_hw_job_finish()
> > > > > below.
> > > > >
> > > > > Also, I'd use a completion here rather than an open coded wait and
> > > > > wake-up. The driver could reinit_completion() before queuing a job to
> > > > > the hardware and the IRQ handler would complete(). There would be no
> > > > > need to store the IRQ flags in driver data anymore.
> > > > Ok, It will be refined as following:
> > > >
> > > > suspend and stop streaming will call mtk_fd_job_abort()
> > > >
> > > > static void mtk_fd_job_abort(struct mtk_fd_dev *fd)
> > > > {
> > > > u32 ret;
> > > >
> > > > ret = wait_for_completion_timeout(&fd->fd_irq_done,
> > > > msecs_to_jiffies(MTK_FD_HW_TIMEOUT));
> > > > if (ret)
> > >
> > > For the _timeout variants, !ret means the timeout and ret > 0 means
> > > that the wait ended successfully before.
> > >
> > Thanks, fixed.
> >
> > > Also please make sure that the timeout is big enough not to happen
> > > normally on a heavily loaded system. Something like 1 second should be
> > > good.
> > >
> > Ok, I will make it 1 second
> > #define MTK_FD_HW_TIMEOUT 1000
> >
> > Also, I will add a condition in mtk_fd_vb2_stop_streaming(), since it
> > would be called twice, now it works fine whether I reuse the condition
> > with mtk_fd_hw_disconnect or not, however, I think do it before buffer
> > remove looks more reasonable.
> >
> > static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq)
> > {
> > struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > struct mtk_fd_dev *fd = ctx->fd_dev;
> > struct vb2_v4l2_buffer *vb;
> > struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> > struct v4l2_m2m_queue_ctx *queue_ctx;
> >
> > if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> > mtk_fd_job_abort(fd);
>
> We need to do it regardless of the queue type, otherwise we could
> still free CAPTURE buffers before the hardware releases them.
>
I think we may read the fd->fd_irq_done.done and do wait for completion
if it's not being done.
How do you think?
static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq)
{
struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
struct mtk_fd_dev *fd = ctx->fd_dev;
struct vb2_v4l2_buffer *vb;
struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
struct v4l2_m2m_queue_ctx *queue_ctx;
u32 ret;
if (!fd->fd_irq_done.done)
ret = wait_for_completion_timeout(&fd->fd_irq_done,
msecs_to_jiffies(
MTK_FD_HW_TIMEOUT));
queue_ctx = V4L2_TYPE_IS_OUTPUT(vq->type) ?
&m2m_ctx->out_q_ctx :
&m2m_ctx->cap_q_ctx;
while ((vb = v4l2_m2m_buf_remove(queue_ctx)))
v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR);
if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
mtk_fd_hw_disconnect(fd);
}
I've also tried to wait completion unconditionally for both queues and
the second time will wait until timeout, as a result, it takes longer to
swap the camera every time and close the camera app.
Thanks and best regards,
Jerry
> Best regards,
> Tomasz
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH] firmware: imx: warn on unexpected RX
From: Anson Huang @ 2019-09-04 8:00 UTC (permalink / raw)
To: Leonard Crestez, Aisheng Dong, Shawn Guo
Cc: Fabio Estevam, Jassi Brar, dl-linux-imx, kernel@pengutronix.de,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <ddd1f5cd5341db0ca347259953135eaf9e782873.1567583496.git.leonard.crestez@nxp.com>
> Subject: [PATCH] firmware: imx: warn on unexpected RX
>
> The imx_scu_call_rpc function function returns the result inside the same
> "msg" struct containing the transmitted message. This is implemented by
> holding a pointer to msg (which is usually on the stack) in sc_imx_rpc and
> writing to it from imx_scu_rx_callback.
>
> This means that if the have_resp parameter is incorrect or SCU sends an
> unexpected for any reason the most likely result is kernel stack corruption.
>
> Fix this by only setting sc_imx_rpc.msg for the duration of the
> imx_scu_call_rpc call and warning in imx_scu_rx_callback if unset.
>
> Print the unexpected response data to help debugging.
>
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Acked-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> drivers/firmware/imx/imx-scu.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-
> scu.c index 04a24a863d6e..869be7a5172c 100644
> --- a/drivers/firmware/imx/imx-scu.c
> +++ b/drivers/firmware/imx/imx-scu.c
> @@ -105,10 +105,16 @@ static void imx_scu_rx_callback(struct mbox_client
> *c, void *msg)
> struct imx_sc_chan *sc_chan = container_of(c, struct imx_sc_chan,
> cl);
> struct imx_sc_ipc *sc_ipc = sc_chan->sc_ipc;
> struct imx_sc_rpc_msg *hdr;
> u32 *data = msg;
>
> + if (!sc_ipc->msg) {
> + dev_warn(sc_ipc->dev, "unexpected rx idx %d 0x%08x,
> ignore!\n",
> + sc_chan->idx, *data);
> + return;
> + }
> +
> if (sc_chan->idx == 0) {
> hdr = msg;
> sc_ipc->rx_size = hdr->size;
> dev_dbg(sc_ipc->dev, "msg rx size %u\n", sc_ipc->rx_size);
> if (sc_ipc->rx_size > 4)
> @@ -163,11 +169,12 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc,
> void *msg, bool have_resp)
> return -EINVAL;
>
> mutex_lock(&sc_ipc->lock);
> reinit_completion(&sc_ipc->done);
>
> - sc_ipc->msg = msg;
> + if (have_resp)
> + sc_ipc->msg = msg;
> sc_ipc->count = 0;
> ret = imx_scu_ipc_write(sc_ipc, msg);
> if (ret < 0) {
> dev_err(sc_ipc->dev, "RPC send msg failed: %d\n", ret);
> goto out;
> @@ -185,10 +192,11 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc,
> void *msg, bool have_resp)
> hdr = msg;
> ret = hdr->func;
> }
>
> out:
> + sc_ipc->msg = NULL;
> mutex_unlock(&sc_ipc->lock);
>
> dev_dbg(sc_ipc->dev, "RPC SVC done\n");
>
> return imx_sc_to_linux_errno(ret);
> --
> 2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] firmware: imx: warn on unexpected RX
From: Leonard Crestez @ 2019-09-04 7:54 UTC (permalink / raw)
To: Anson Huang, Dong Aisheng, Shawn Guo
Cc: Fabio Estevam, Jassi Brar, linux-imx, kernel, linux-arm-kernel
The imx_scu_call_rpc function function returns the result inside the
same "msg" struct containing the transmitted message. This is
implemented by holding a pointer to msg (which is usually on the stack)
in sc_imx_rpc and writing to it from imx_scu_rx_callback.
This means that if the have_resp parameter is incorrect or SCU sends an
unexpected for any reason the most likely result is kernel stack
corruption.
Fix this by only setting sc_imx_rpc.msg for the duration of the
imx_scu_call_rpc call and warning in imx_scu_rx_callback if unset.
Print the unexpected response data to help debugging.
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
drivers/firmware/imx/imx-scu.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
index 04a24a863d6e..869be7a5172c 100644
--- a/drivers/firmware/imx/imx-scu.c
+++ b/drivers/firmware/imx/imx-scu.c
@@ -105,10 +105,16 @@ static void imx_scu_rx_callback(struct mbox_client *c, void *msg)
struct imx_sc_chan *sc_chan = container_of(c, struct imx_sc_chan, cl);
struct imx_sc_ipc *sc_ipc = sc_chan->sc_ipc;
struct imx_sc_rpc_msg *hdr;
u32 *data = msg;
+ if (!sc_ipc->msg) {
+ dev_warn(sc_ipc->dev, "unexpected rx idx %d 0x%08x, ignore!\n",
+ sc_chan->idx, *data);
+ return;
+ }
+
if (sc_chan->idx == 0) {
hdr = msg;
sc_ipc->rx_size = hdr->size;
dev_dbg(sc_ipc->dev, "msg rx size %u\n", sc_ipc->rx_size);
if (sc_ipc->rx_size > 4)
@@ -163,11 +169,12 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp)
return -EINVAL;
mutex_lock(&sc_ipc->lock);
reinit_completion(&sc_ipc->done);
- sc_ipc->msg = msg;
+ if (have_resp)
+ sc_ipc->msg = msg;
sc_ipc->count = 0;
ret = imx_scu_ipc_write(sc_ipc, msg);
if (ret < 0) {
dev_err(sc_ipc->dev, "RPC send msg failed: %d\n", ret);
goto out;
@@ -185,10 +192,11 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp)
hdr = msg;
ret = hdr->func;
}
out:
+ sc_ipc->msg = NULL;
mutex_unlock(&sc_ipc->lock);
dev_dbg(sc_ipc->dev, "RPC SVC done\n");
return imx_sc_to_linux_errno(ret);
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 2/2] mmc: block: add CMD13 polling for ioctl() cmd with R1B response
From: Chaotian Jing @ 2019-09-04 7:54 UTC (permalink / raw)
To: Ulf Hansson
Cc: Jens Axboe, Chris Boot, srv_heupstream, Wolfram Sang, linux-mmc,
Zachary Hays, YueHaibing, linux-kernel, Ming Lei, Avri Altman,
linux-mediatek, Hannes Reinecke, Matthias Brugger, Chaotian Jing,
linux-arm-kernel
In-Reply-To: <20190904075444.2163-1-chaotian.jing@mediatek.com>
currently there is no CMD13 polling and other code to wait card
change to transfer state after R1B command completed. and this
polling operation cannot do in user space, because other request
may coming before the CMD13 from user space.
Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
---
drivers/mmc/core/block.c | 107 ++++++++++++++++++++-------------------
1 file changed, 55 insertions(+), 52 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index aa7c19f7e298..9d6f7a5612b5 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -468,6 +468,58 @@ static int ioctl_do_sanitize(struct mmc_card *card)
return err;
}
+static inline bool mmc_blk_in_tran_state(u32 status)
+{
+ /*
+ * Some cards mishandle the status bits, so make sure to check both the
+ * busy indication and the card state.
+ */
+ return status & R1_READY_FOR_DATA &&
+ (R1_CURRENT_STATE(status) == R1_STATE_TRAN);
+}
+
+static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms,
+ u32 *resp_errs)
+{
+ unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms);
+ int err = 0;
+ u32 status;
+
+ do {
+ bool done = time_after(jiffies, timeout);
+
+ err = __mmc_send_status(card, &status, 5);
+ if (err) {
+ dev_err(mmc_dev(card->host),
+ "error %d requesting status\n", err);
+ return err;
+ }
+
+ /* Accumulate any response error bits seen */
+ if (resp_errs)
+ *resp_errs |= status;
+
+ /*
+ * Timeout if the device never becomes ready for data and never
+ * leaves the program state.
+ */
+ if (done) {
+ dev_err(mmc_dev(card->host),
+ "Card stuck in wrong state! %s status: %#x\n",
+ __func__, status);
+ return -ETIMEDOUT;
+ }
+
+ /*
+ * Some cards mishandle the status bits,
+ * so make sure to check both the busy
+ * indication and the card state.
+ */
+ } while (!mmc_blk_in_tran_state(status));
+
+ return err;
+}
+
static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
struct mmc_blk_ioc_data *idata)
{
@@ -623,6 +675,9 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
__func__, status, err);
}
+ if (!err && (cmd.flags & MMC_RSP_R1B))
+ err = card_busy_detect(card, MMC_BLK_TIMEOUT_MS, NULL);
+
return err;
}
@@ -970,58 +1025,6 @@ static unsigned int mmc_blk_data_timeout_ms(struct mmc_host *host,
return ms;
}
-static inline bool mmc_blk_in_tran_state(u32 status)
-{
- /*
- * Some cards mishandle the status bits, so make sure to check both the
- * busy indication and the card state.
- */
- return status & R1_READY_FOR_DATA &&
- (R1_CURRENT_STATE(status) == R1_STATE_TRAN);
-}
-
-static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms,
- u32 *resp_errs)
-{
- unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms);
- int err = 0;
- u32 status;
-
- do {
- bool done = time_after(jiffies, timeout);
-
- err = __mmc_send_status(card, &status, 5);
- if (err) {
- dev_err(mmc_dev(card->host),
- "error %d requesting status\n", err);
- return err;
- }
-
- /* Accumulate any response error bits seen */
- if (resp_errs)
- *resp_errs |= status;
-
- /*
- * Timeout if the device never becomes ready for data and never
- * leaves the program state.
- */
- if (done) {
- dev_err(mmc_dev(card->host),
- "Card stuck in wrong state! %s status: %#x\n",
- __func__, status);
- return -ETIMEDOUT;
- }
-
- /*
- * Some cards mishandle the status bits,
- * so make sure to check both the busy
- * indication and the card state.
- */
- } while (!mmc_blk_in_tran_state(status));
-
- return err;
-}
-
static int mmc_blk_reset(struct mmc_blk_data *md, struct mmc_host *host,
int type)
{
--
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* fix device in programming state after ioctl()
From: Chaotian Jing @ 2019-09-04 7:54 UTC (permalink / raw)
To: Ulf Hansson
Cc: Jens Axboe, Chris Boot, srv_heupstream, Wolfram Sang, linux-mmc,
Zachary Hays, YueHaibing, linux-kernel, Ming Lei, Avri Altman,
linux-mediatek, Hannes Reinecke, Matthias Brugger, Chaotian Jing,
linux-arm-kernel
the user space program may access eMMC by ioctl(), after the ioctl() was
completed, it should ensure that eMMC is in transfer state, or it will
cause other thread which access eMMC got timeout error, as it assume that
card was in transfer state.
this patch add CMD13 polling for R1B command to avoid this issue.
Chaotian Jing (2):
mmc: block: make the card_busy_detect() more generic
mmc: block: add CMD13 polling for ioctl() cmd with R1B response
drivers/mmc/core/block.c | 111 ++++++++++++++++++++-------------------
1 file changed, 57 insertions(+), 54 deletions(-)
--
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 1/2] mmc: block: make the card_busy_detect() more generic
From: Chaotian Jing @ 2019-09-04 7:54 UTC (permalink / raw)
To: Ulf Hansson
Cc: Jens Axboe, Chris Boot, srv_heupstream, Wolfram Sang, linux-mmc,
Zachary Hays, YueHaibing, linux-kernel, Ming Lei, Avri Altman,
linux-mediatek, Hannes Reinecke, Matthias Brugger, Chaotian Jing,
linux-arm-kernel
In-Reply-To: <20190904075444.2163-1-chaotian.jing@mediatek.com>
to use the card_busy_detect() to wait card levae the programming state,
there may be do not have the "struct request *" argument.
Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
---
drivers/mmc/core/block.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 2c71a434c915..aa7c19f7e298 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -981,7 +981,7 @@ static inline bool mmc_blk_in_tran_state(u32 status)
}
static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms,
- struct request *req, u32 *resp_errs)
+ u32 *resp_errs)
{
unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms);
int err = 0;
@@ -992,8 +992,8 @@ static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms,
err = __mmc_send_status(card, &status, 5);
if (err) {
- pr_err("%s: error %d requesting status\n",
- req->rq_disk->disk_name, err);
+ dev_err(mmc_dev(card->host),
+ "error %d requesting status\n", err);
return err;
}
@@ -1006,9 +1006,9 @@ static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms,
* leaves the program state.
*/
if (done) {
- pr_err("%s: Card stuck in wrong state! %s %s status: %#x\n",
- mmc_hostname(card->host),
- req->rq_disk->disk_name, __func__, status);
+ dev_err(mmc_dev(card->host),
+ "Card stuck in wrong state! %s status: %#x\n",
+ __func__, status);
return -ETIMEDOUT;
}
@@ -1671,7 +1671,7 @@ static int mmc_blk_fix_state(struct mmc_card *card, struct request *req)
mmc_blk_send_stop(card, timeout);
- err = card_busy_detect(card, timeout, req, NULL);
+ err = card_busy_detect(card, timeout, NULL);
mmc_retune_release(card->host);
@@ -1895,7 +1895,7 @@ static int mmc_blk_card_busy(struct mmc_card *card, struct request *req)
if (mmc_host_is_spi(card->host) || rq_data_dir(req) == READ)
return 0;
- err = card_busy_detect(card, MMC_BLK_TIMEOUT_MS, req, &status);
+ err = card_busy_detect(card, MMC_BLK_TIMEOUT_MS, &status);
/*
* Do not assume data transferred correctly if there are any error bits
--
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v3 4/7] serial: fsl_linflexuart: Be consistent with the name
From: gregkh @ 2019-09-04 7:52 UTC (permalink / raw)
To: Stefan-gabriel Mirea
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, corbet@lwn.net,
catalin.marinas@arm.com, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, Leo Li, robh+dt@kernel.org,
linux-serial@vger.kernel.org, jslaby@suse.com,
shawnguo@kernel.org, will@kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190823191115.18490-5-stefan-gabriel.mirea@nxp.com>
On Fri, Aug 23, 2019 at 07:11:37PM +0000, Stefan-gabriel Mirea wrote:
> For consistency reasons, spell the controller name as "LINFlexD" in
> comments and documentation.
>
> Signed-off-by: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 2 +-
> drivers/tty/serial/Kconfig | 8 ++++----
> drivers/tty/serial/fsl_linflexuart.c | 4 ++--
> include/uapi/linux/serial_core.h | 4 ++--
> 4 files changed, 9 insertions(+), 9 deletions(-)
Doesn't apply to my tty-next tree :(
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] i2c: stm32f7: Make structure stm32f7_i2c_algo constant
From: Pierre Yves MORDRET @ 2019-09-04 7:47 UTC (permalink / raw)
To: Wolfram Sang, Nishka Dasgupta
Cc: alexandre.torgue, linux-kernel, linux-i2c, mcoquelin.stm32,
linux-stm32, linux-arm-kernel
In-Reply-To: <20190903180552.GI2171@ninjato>
Hi Wolfram
Sorry for the delay.
Acked-by: Pierre-Yves MORDRET <pierre-yves.mordret@st.com>
BR
On 9/3/19 8:05 PM, Wolfram Sang wrote:
> On Thu, Aug 15, 2019 at 11:28:57AM +0530, Nishka Dasgupta wrote:
>> Static structure stm32f7_i2c_algo, of type i2c_algorithm, is used only
>> when it is assigned to constant field algo of a variable having type
>> i2c_adapter. As stm32f7_i2c_algo is therefore never modified, make it
>> const as well to protect it from unintended modification.
>> Issue found with Coccinelle.
>>
>> Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com>
>> ---
>
> Are you guys okay with this patch?
>
>> drivers/i2c/busses/i2c-stm32f7.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
>> index 266d1c269b83..d36cf08461f7 100644
>> --- a/drivers/i2c/busses/i2c-stm32f7.c
>> +++ b/drivers/i2c/busses/i2c-stm32f7.c
>> @@ -1809,7 +1809,7 @@ static u32 stm32f7_i2c_func(struct i2c_adapter *adap)
>> I2C_FUNC_SMBUS_I2C_BLOCK;
>> }
>>
>> -static struct i2c_algorithm stm32f7_i2c_algo = {
>> +static const struct i2c_algorithm stm32f7_i2c_algo = {
>> .master_xfer = stm32f7_i2c_xfer,
>> .smbus_xfer = stm32f7_i2c_smbus_xfer,
>> .functionality = stm32f7_i2c_func,
>> --
>> 2.19.1
>>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH] soc: imx: imx-scu: Getting UID from SCU should have response
From: Anson Huang @ 2019-09-04 7:41 UTC (permalink / raw)
To: Leonard Crestez, Aisheng Dong
Cc: Abel Vesa, festevam@gmail.com, s.hauer@pengutronix.de,
linux-kernel@vger.kernel.org, dl-linux-imx, kernel@pengutronix.de,
shawnguo@kernel.org, Daniel Baluta,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <VI1PR04MB7023B9C325C54AA8112F1AE5EEB80@VI1PR04MB7023.eurprd04.prod.outlook.com>
Hi, Leonard
> On 2019-09-04 10:14 AM, Anson Huang wrote:
> > The SCU firmware API for getting UID should have response, otherwise,
> > the message stored in function stack could be released and then the
> > response data received from SCU will be stored into that released
> > stack and cause kernel NULL pointer dump.
>
> This fix looks good, but looking at imx-scu code it seems that passing the
> incorrect "have_resp" argument to imx_scu_call_rpc or just receiving an
> unexpected message from SCFW will always result in kernel stack corruption!
>
> This is worth handling inside imx-scu itself: unless a response was explicitly
> requested it should ignore and print a warning on rx, for example by setting
> imx_sc_ipc to NULL when not waiting for a response.
>
> Holding on to arbitrary stack pointers is bad.
We noticed this issue recently during the development of ON/OFF button support,
this UID is lucky, the stack is NOT released when SCU response data is received, but
this fix should be applied.
We talked to Chuck about adding return value for these special APIs, response data
needed but no return value from SCU, so very likely we will need a patch in imx_sc_ipc
driver to enhance/handle that, will do a patch for it later.
Thanks,
Anson
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] soc: imx: imx-scu: Getting UID from SCU should have response
From: Leonard Crestez @ 2019-09-04 7:34 UTC (permalink / raw)
To: Anson Huang, Aisheng Dong
Cc: Abel Vesa, festevam@gmail.com, s.hauer@pengutronix.de,
linux-kernel@vger.kernel.org, dl-linux-imx, kernel@pengutronix.de,
shawnguo@kernel.org, Daniel Baluta,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <1567624394-25023-1-git-send-email-Anson.Huang@nxp.com>
On 2019-09-04 10:14 AM, Anson Huang wrote:
> The SCU firmware API for getting UID should have response,
> otherwise, the message stored in function stack could be
> released and then the response data received from SCU will be
> stored into that released stack and cause kernel NULL pointer
> dump.
This fix looks good, but looking at imx-scu code it seems that passing
the incorrect "have_resp" argument to imx_scu_call_rpc or just receiving
an unexpected message from SCFW will always result in kernel stack
corruption!
This is worth handling inside imx-scu itself: unless a response was
explicitly requested it should ignore and print a warning on rx, for
example by setting imx_sc_ipc to NULL when not waiting for a response.
Holding on to arbitrary stack pointers is bad.
--
Regards,
Leonard
> diff --git a/drivers/soc/imx/soc-imx-scu.c b/drivers/soc/imx/soc-imx-scu.c
> index 50831eb..c68882e 100644
> --- a/drivers/soc/imx/soc-imx-scu.c
> +++ b/drivers/soc/imx/soc-imx-scu.c
> @@ -46,7 +46,7 @@ static ssize_t soc_uid_show(struct device *dev,
> hdr->func = IMX_SC_MISC_FUNC_UNIQUE_ID;
> hdr->size = 1;
>
> - ret = imx_scu_call_rpc(soc_ipc_handle, &msg, false);
> + ret = imx_scu_call_rpc(soc_ipc_handle, &msg, true);
> if (ret) {
> pr_err("%s: get soc uid failed, ret %d\n", __func__, ret);
> return ret;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v5] perf machine: arm/arm64: Improve completeness for kernel address space
From: Adrian Hunter @ 2019-09-04 7:26 UTC (permalink / raw)
To: Leo Yan
Cc: Song Liu, Mathieu Poirier, Daniel Borkmann, Suzuki Poulouse,
Alexander Shishkin, netdev, coresight, Alexei Starovoitov,
Arnaldo Carvalho de Melo, linux-kernel, clang-built-linux,
Peter Zijlstra, Yonghong Song, Namhyung Kim, bpf, Jiri Olsa,
Martin KaFai Lau, linux-arm-kernel
In-Reply-To: <20190902141511.GF4931@leoy-ThinkPad-X240s>
On 2/09/19 5:15 PM, Leo Yan wrote:
> Hi Adrian,
>
> On Mon, Aug 26, 2019 at 08:51:05PM +0800, Leo Yan wrote:
>> Hi Adrian,
>>
>> On Fri, Aug 16, 2019 at 04:00:02PM +0300, Adrian Hunter wrote:
>>> On 16/08/19 4:45 AM, Leo Yan wrote:
>>>> Hi Adrian,
>>>>
>>>> On Thu, Aug 15, 2019 at 02:45:57PM +0300, Adrian Hunter wrote:
>>>>
>>>> [...]
>>>>
>>>>>>> How come you cannot use kallsyms to get the information?
>>>>>>
>>>>>> Thanks for pointing out this. Sorry I skipped your comment "I don't
>>>>>> know how you intend to calculate ARM_PRE_START_SIZE" when you reviewed
>>>>>> the patch v3, I should use that chance to elaborate the detailed idea
>>>>>> and so can get more feedback/guidance before procceed.
>>>>>>
>>>>>> Actually, I have considered to use kallsyms when worked on the previous
>>>>>> patch set.
>>>>>>
>>>>>> As mentioned in patch set v4's cover letter, I tried to implement
>>>>>> machine__create_extra_kernel_maps() for arm/arm64, the purpose is to
>>>>>> parse kallsyms so can find more kernel maps and thus also can fixup
>>>>>> the kernel start address. But I found the 'perf script' tool directly
>>>>>> calls machine__get_kernel_start() instead of running into the flow for
>>>>>> machine__create_extra_kernel_maps();
>>>>>
>>>>> Doesn't it just need to loop through each kernel map to find the lowest
>>>>> start address?
>>>>
>>>> Based on your suggestion, I worked out below change and verified it
>>>> can work well on arm64 for fixing up start address; please let me know
>>>> if the change works for you?
>>>
>>> How does that work if take a perf.data file to a machine with a different
>>> architecture?
>>
>> Sorry I delayed so long to respond to your question; I didn't have
>> confidence to give out very reasonale answer and this is the main reason
>> for delaying.
>
> Could you take chance to review my below replying? I'd like to get
> your confirmation before I send out offical patch.
It is not necessary to do kallsyms__parse for x86_64, so it would be better
to check the arch before calling that.
However in general, having to copy and use kallsyms with perf.data if on a
different arch does not seem very user friendly. But really that is up to
Arnaldo.
>
> Thanks,
> Leo Yan
>
>>
>> For your question for taking a perf.data file to a machine with a
>> different architecture, we can firstly use command 'perf buildid-list'
>> to print out the buildid for kallsyms, based on the dumped buildid we
>> can find out the location for the saved kallsyms file; then we can use
>> option '--kallsyms' to specify the offline kallsyms file and use the
>> offline kallsyms to fixup kernel start address. The detailed commands
>> are listed as below:
>>
>> root@debian:~# perf buildid-list
>> 7b36dfca8317ef74974ebd7ee5ec0a8b35c97640 [kernel.kallsyms]
>> 56b84aa88a1bcfe222a97a53698b92723a3977ca /usr/lib/systemd/systemd
>> 0956b952e9cd673d48ff2cfeb1a9dbd0c853e686 /usr/lib/aarch64-linux-gnu/libm-2.28.so
>> [...]
>>
>> root@debian:~# perf script --kallsyms ~/.debug/\[kernel.kallsyms\]/7b36dfca8317ef74974ebd7ee5ec0a8b35c97640/kallsyms
>>
>> The amended patch is as below, please review and always welcome
>> any suggestions or comments!
>>
>> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
>> index 5734460fc89e..593f05cc453f 100644
>> --- a/tools/perf/util/machine.c
>> +++ b/tools/perf/util/machine.c
>> @@ -2672,9 +2672,26 @@ int machine__nr_cpus_avail(struct machine *machine)
>> return machine ? perf_env__nr_cpus_avail(machine->env) : 0;
>> }
>>
>> +static int machine__fixup_kernel_start(void *arg,
>> + const char *name __maybe_unused,
>> + char type,
>> + u64 start)
>> +{
>> + struct machine *machine = arg;
>> +
>> + type = toupper(type);
>> +
>> + /* Fixup for text, weak, data and bss sections. */
>> + if (type == 'T' || type == 'W' || type == 'D' || type == 'B')
>> + machine->kernel_start = min(machine->kernel_start, start);
>> +
>> + return 0;
>> +}
>> +
>> int machine__get_kernel_start(struct machine *machine)
>> {
>> struct map *map = machine__kernel_map(machine);
>> + char filename[PATH_MAX];
>> int err = 0;
>>
>> /*
>> @@ -2696,6 +2713,22 @@ int machine__get_kernel_start(struct machine *machine)
>> if (!err && !machine__is(machine, "x86_64"))
>> machine->kernel_start = map->start;
>> }
>> +
>> + if (symbol_conf.kallsyms_name != NULL) {
>> + strncpy(filename, symbol_conf.kallsyms_name, PATH_MAX);
>> + } else {
>> + machine__get_kallsyms_filename(machine, filename, PATH_MAX);
>> +
>> + if (symbol__restricted_filename(filename, "/proc/kallsyms"))
>> + goto out;
>> + }
>> +
>> + if (kallsyms__parse(filename, machine, machine__fixup_kernel_start))
>> + pr_warning("Fail to fixup kernel start address. skipping...\n");
>> +
>> +out:
>> return err;
>> }
>>
>>
>> Thanks,
>> Leo Yan
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PULL 2/5] Renesas ARM SoC updates for v5.4
From: Geert Uytterhoeven @ 2019-09-04 7:15 UTC (permalink / raw)
To: Arnd Bergmann
Cc: arm-soc, Geert Uytterhoeven, Magnus Damm, Linux-Renesas, arm-soc,
Simon Horman, Linux ARM
In-Reply-To: <CAK8P3a11EfOXfwZ5Xx3vYJwfBGPh=yX73f_=3u7Zmm+hJF6HVg@mail.gmail.com>
Hi Arnd,
On Tue, Sep 3, 2019 at 11:15 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Aug 23, 2019 at 2:36 PM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> > Renesas ARM SoC updates for v5.4
> >
> > - Low-level debugging support for RZ/A2M.
> >
>
> Pulled into arm/soc, thanks!
>
> This should be the last of your pull requests for the moment. Can you check that
> everything is there once it hits linux-next (probably Wednesday, I may have just
> missed today).
$ for i in $(git tag | grep renesas-.*-for-v5.4); do echo --- $i ---;
git cherry -v arm-soc/for-next $i; done
--- renesas-arm-dt-for-v5.4-tag1 ---
--- renesas-arm-soc-for-v5.4-tag1 ---
--- renesas-arm64-dt-for-v5.4-tag1 ---
--- renesas-arm64-dt-for-v5.4-tag2 ---
--- renesas-drivers-for-v5.4-tag1 ---
--- renesas-drivers-for-v5.4-tag2 ---
--- renesas-dt-bindings-for-v5.4-tag1 ---
--- renesas-dt-bindings-for-v5.4-tag2 ---
Yep, all included.
Thanks a lot!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] soc: imx: imx-scu: Getting UID from SCU should have response
From: Anson Huang @ 2019-09-04 19:13 UTC (permalink / raw)
To: shawnguo, s.hauer, kernel, festevam, daniel.baluta, aisheng.dong,
abel.vesa, linux-arm-kernel, linux-kernel
Cc: Linux-imx
The SCU firmware API for getting UID should have response,
otherwise, the message stored in function stack could be
released and then the response data received from SCU will be
stored into that released stack and cause kernel NULL pointer
dump.
Fixes: 73feb4d0f8f1 ("soc: imx-scu: Add SoC UID(unique identifier) support")
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
drivers/soc/imx/soc-imx-scu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soc/imx/soc-imx-scu.c b/drivers/soc/imx/soc-imx-scu.c
index 50831eb..c68882e 100644
--- a/drivers/soc/imx/soc-imx-scu.c
+++ b/drivers/soc/imx/soc-imx-scu.c
@@ -46,7 +46,7 @@ static ssize_t soc_uid_show(struct device *dev,
hdr->func = IMX_SC_MISC_FUNC_UNIQUE_ID;
hdr->size = 1;
- ret = imx_scu_call_rpc(soc_ipc_handle, &msg, false);
+ ret = imx_scu_call_rpc(soc_ipc_handle, &msg, true);
if (ret) {
pr_err("%s: get soc uid failed, ret %d\n", __func__, ret);
return ret;
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v3 0/7] add support USB for MT8183
From: Greg Kroah-Hartman @ 2019-09-04 7:06 UTC (permalink / raw)
To: Chunfeng Yun
Cc: Mark Rutland, devicetree, Mathias Nyman, linux-usb, linux-kernel,
Rob Herring, linux-mediatek, Matthias Brugger, linux-arm-kernel
In-Reply-To: <1567562067.7317.52.camel@mhfsdcap03>
On Wed, Sep 04, 2019 at 09:54:27AM +0800, Chunfeng Yun wrote:
> Hi Greg,
>
>
> Please don't try to pick up this series, the dependent ones are still
> under public review, I'll fix build warning and send out new version
> after the dependent ones are applied
> Sorry for inconvenience
No problem, now dropped from my review queue.
greg k-h
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 4/4] gpio: Update documentation with ast2600 controllers
From: Bartosz Golaszewski @ 2019-09-04 7:02 UTC (permalink / raw)
To: Rashmica Gupta
Cc: linux-aspeed, Andrew Jeffery, Linus Walleij, LKML, linux-gpio,
Joel Stanley, arm-soc
In-Reply-To: <20190904061245.30770-4-rashmica.g@gmail.com>
śr., 4 wrz 2019 o 08:13 Rashmica Gupta <rashmica.g@gmail.com> napisał(a):
>
Again, this needs a proper commit description and the subject should
start with "dt-bindings: ...".
You also need to Cc the device-tree maintainers. Use
scripts/get_maintainer.pl to list all people that should get this
patch.
Bart
> Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
> ---
> Documentation/devicetree/bindings/gpio/gpio-aspeed.txt | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-aspeed.txt b/Documentation/devicetree/bindings/gpio/gpio-aspeed.txt
> index 7e9b586770b0..cd388797e07c 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-aspeed.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-aspeed.txt
> @@ -2,7 +2,8 @@ Aspeed GPIO controller Device Tree Bindings
> -------------------------------------------
>
> Required properties:
> -- compatible : Either "aspeed,ast2400-gpio" or "aspeed,ast2500-gpio"
> +- compatible : Either "aspeed,ast2400-gpio", "aspeed,ast2500-gpio",
> + "aspeed,ast2600-gpio", or "aspeed,ast2600-1-8v-gpio"
>
> - #gpio-cells : Should be two
> - First cell is the GPIO line number
> --
> 2.20.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 08/13] swiotlb-xen: always use dma-direct helpers to alloc coherent pages
From: Christoph Hellwig @ 2019-09-04 7:00 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Juergen Gross, Stefano Stabellini, Konrad Rzeszutek Wilk, x86,
linux-kernel, iommu, xen-devel, Christoph Hellwig,
linux-arm-kernel
In-Reply-To: <8a7f9e23-ef26-e83b-8d1e-dcd7d233642a@oracle.com>
On Tue, Sep 03, 2019 at 06:25:54PM -0400, Boris Ostrovsky wrote:
> > If I am reading __dma_direct_alloc_pages() correctly there is a path
> > that will force us to use GFP_DMA32 and as Juergen pointed out in
> > another message that may not be desirable.
Yes, it will add GFP_DMA32. So I guess for now we'll just keep the
direct page allocator calls and resend. I don't think this is the
right thing to do long term, but I'd really like to get this series
finished.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/4] gpio/aspeed: Fix incorrect number of banks
From: Bartosz Golaszewski @ 2019-09-04 6:57 UTC (permalink / raw)
To: Rashmica Gupta
Cc: linux-aspeed, Andrew Jeffery, Linus Walleij, LKML, linux-gpio,
Joel Stanley, arm-soc
In-Reply-To: <20190904061245.30770-1-rashmica.g@gmail.com>
śr., 4 wrz 2019 o 08:13 Rashmica Gupta <rashmica.g@gmail.com> napisał(a):
>
> Fixes: 361b79119a4b7 ('gpio: Add Aspeed driver')
>
Please, add a proper commit description. Checkpatch should have warned
you about it.
Bart
> Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
> ---
> drivers/gpio/gpio-aspeed.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
> index 9defe25d4721..77752b2624e8 100644
> --- a/drivers/gpio/gpio-aspeed.c
> +++ b/drivers/gpio/gpio-aspeed.c
> @@ -1165,7 +1165,7 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)
> gpio->chip.base = -1;
>
> /* Allocate a cache of the output registers */
> - banks = gpio->config->nr_gpios >> 5;
> + banks = (gpio->config->nr_gpios >> 5) + 1;
> gpio->dcache = devm_kcalloc(&pdev->dev,
> banks, sizeof(u32), GFP_KERNEL);
> if (!gpio->dcache)
> --
> 2.20.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox