linux-kernel - Re: [PATCH v5 18/22] media: platform: Add Sunxi-Cedrus VPU decoder driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <25fee2b5ad35f46ccadf56b93d1a9261c9855f97.camel@bootlin.com>
Date:   Tue, 24 Jul 2018 16:56:57 +0200
From:   Paul Kocialkowski <paul.kocialkowski@...tlin.com>
To:     Maxime Ripard <maxime.ripard@...tlin.com>
Cc:     linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Chen-Yu Tsai <wens@...e.org>,
        Marco Franchi <marco.franchi@....com>,
        Icenowy Zheng <icenowy@...c.io>,
        Hans Verkuil <hverkuil@...all.nl>,
        Keiichi Watanabe <keiichiw@...omium.org>,
        Jonathan Corbet <corbet@....net>,
        Smitha T Murthy <smitha.t@...sung.com>,
        Tom Saeger <tom.saeger@...cle.com>,
        Andrzej Hajda <a.hajda@...sung.com>,
        "David S . Miller" <davem@...emloft.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        Arnd Bergmann <arnd@...db.de>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
        Jacob Chen <jacob-chen@...wrt.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Benoit Parrot <bparrot@...com>,
        Todor Tomov <todor.tomov@...aro.org>,
        Alexandre Courbot <acourbot@...omium.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Pawel Osciak <posciak@...omium.org>,
        Ricardo Ribalda Delgado <ricardo.ribalda@...il.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Sami Tolvanen <samitolvanen@...gle.com>,
        Niklas Söderlund 
        <niklas.soderlund+renesas@...natech.se>,
        linux-sunxi@...glegroups.com,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Hugues Fruchet <hugues.fruchet@...com>,
        Randy Li <ayaka@...lik.info>
Subject: Re: [PATCH v5 18/22] media: platform: Add Sunxi-Cedrus VPU decoder
 driver

Hi,

On Tue, 2018-07-10 at 10:42 +0200, Maxime Ripard wrote:
> On Tue, Jul 10, 2018 at 10:01:10AM +0200, Paul Kocialkowski wrote:
> > +static int cedrus_remove(struct platform_device *pdev)
> > +{
> > +       struct cedrus_dev *dev = platform_get_drvdata(pdev);
> > +
> > +       v4l2_info(&dev->v4l2_dev, "Removing " CEDRUS_NAME);
> 
> That log is kind of pointless.

Fair enough, I'll get rid of it.

> > +static void cedrus_hw_set_capabilities(struct cedrus_dev *dev)
> > +{
> > +	unsigned int engine_version;
> > +
> > +	engine_version = cedrus_read(dev, VE_VERSION) >> VE_VERSION_SHIFT;
> > +
> > +	if (engine_version >= 0x1667)
> > +		dev->capabilities |= CEDRUS_CAPABILITY_UNTILED;
> 
> The version used here would need a define, but I'm wondering if this
> is the right solution here. You are using at the same time the version
> ID returned by the register and the compatible in various places, and
> they are both redundant. If you want to base the capabilities on the
> compatible, then you can do it for all of those properties and
> capabilities, and if you want to use the version register, then you
> don't need all those compatibles but just one.
> 
> I think that basing all our capabilities on the compatible makes more
> sense, since you need to have access to the registers in order to read
> the version register, and this changes from one SoC generation to the
> other (for example, keeping the reset line asserted would prevent you
> from reading it, and the fact that there is a reset line depends on
> the SoC).

I concur, let's move this to a compatible-based logic instead!

> > +int cedrus_hw_probe(struct cedrus_dev *dev)
> > +{
> > +	struct resource *res;
> > +	int irq_dec;
> > +	int ret;
> > +
> > +	irq_dec = platform_get_irq(dev->pdev, 0);
> > +	if (irq_dec <= 0) {
> > +		v4l2_err(&dev->v4l2_dev, "Failed to get IRQ\n");
> > +		return -ENXIO;
> > +	}
> 
> You already have an error code returned by platform_get_irq, there's
> no point in masking it and returning -ENXIO. This can even lead to
> some bugs, for example when the error code is EPROBE_DEFER.

Right, I'll fix this and all the similar issues you mentionned.

[...]

> There's also a bunch of warnings/checks reported by checkpatch that
> should be fixed in the next iteration: the spaces after a cast, the
> NULL comparison, macros arguments precedence, parenthesis alignments
> issues, etc.)

Thanks, I'll look into that for the next revision.

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)