| 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
| ||
|
Message-ID: <CAK8P3a1+CZTAcR5T=gN565Q8=CdZnu5KYsAijKXLY8taofEpGg@mail.gmail.com>
Date: Fri, 26 Feb 2021 11:37:44 +0100
From: Arnd Bergmann <arnd@...nel.org>
To: Fabrizio Castro <fabrizio.castro.jz@...esas.com>
Cc: Rob Herring <robh+dt@...nel.org>, Arnd Bergmann <arnd@...db.de>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Geert Uytterhoeven <geert+renesas@...der.be>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
DTML <devicetree@...r.kernel.org>,
Linux ARM <linux-arm-kernel@...ts.infradead.org>,
Linux API <linux-api@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>,
Chris Paterson <Chris.Paterson2@...esas.com>,
Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...renesas.com>,
Phil Edworthy <phil.edworthy@...esas.com>,
Dirk Behme <Dirk.Behme@...bosch.com>,
Peter Erben <Peter.Erben@...bosch.com>,
Linux Media Mailing List <linux-media@...r.kernel.org>
Subject: Re: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas R-Car devices
On Thu, Feb 25, 2021 at 11:51 PM Fabrizio Castro
<fabrizio.castro.jz@...esas.com> wrote:
>
> The DAB hardware accelerator found on R-Car E3 and R-Car M3-N devices is
> a hardware accelerator for software DAB demodulators.
> It consists of one FFT (Fast Fourier Transform) module and one decoder
> module, compatible with DAB specification (ETSI EN 300 401 and
> ETSI TS 102 563).
> The decoder module can perform FIC decoding and MSC decoding processing
> from de-puncture to final decoded result.
>
> This patch adds a device driver to support the FFT module only.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@...esas.com>
> ---
> MAINTAINERS | 7 ++
> drivers/misc/Kconfig | 1 +
> drivers/misc/Makefile | 1 +
> drivers/misc/rcar_dab/Kconfig | 11 ++
> drivers/misc/rcar_dab/Makefile | 8 ++
> drivers/misc/rcar_dab/rcar_dev.c | 176 +++++++++++++++++++++++++++++++
> drivers/misc/rcar_dab/rcar_dev.h | 116 ++++++++++++++++++++
> drivers/misc/rcar_dab/rcar_fft.c | 160 ++++++++++++++++++++++++++++
> include/uapi/linux/rcar_dab.h | 35 ++++++
Can you explain why this is not in drivers/media/?
I don't think we want a custom ioctl interface for a device that implements
a generic specification. My first feeling would be that this should not
have a user-level API but instead get called by the DAB radio driver.
What is the intended usage model here? I assume the idea is to
use it in an application that receives audio or metadata from DAB.
What driver do you use for that?
> +static long rcar_dab_unlocked_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + void __user *argp = (void __user *)arg;
> + struct rcar_dab *dab;
> + int ret;
> +
> + dab = container_of(file->private_data, struct rcar_dab, misc);
> +
> + switch (cmd) {
> + case RCAR_DAB_IOC_FFT:
> + if (!access_ok(argp, sizeof(struct rcar_dab_fft_req)))
> + return -EFAULT;
> + ret = rcar_dab_fft(dab, argp);
> + break;
> + default:
> + ret = -ENOTTY;
> + }
> +
> + return ret;
> +}
> +
> +static const struct file_operations rcar_dab_fops = {
> + .owner = THIS_MODULE,
> + .unlocked_ioctl = rcar_dab_unlocked_ioctl,
> +};
There should be a '.compat_ioctl = compat_ptr_ioctl'
entry, provided that the arguments are compatible between
32-bit and 64-bit user space.
> +
> +static int rcar_dab_fft_init(struct rcar_dab *dab, struct rcar_dab_fft_req *fft)
> +{
> + u32 mode;
> +
> + for (mode = 0; mode < ARRAY_SIZE(rcar_dab_fft_size_lut); mode++)
> + if (rcar_dab_fft_size_lut[mode] == fft->points)
> + break;
> + if (mode == ARRAY_SIZE(rcar_dab_fft_size_lut))
> + return -EINVAL;
> + if (fft->ofdm_number == 0)
> + return -EINVAL;
> +
> + rcar_dab_write(dab, RCAR_DAB_FFTSSR, mode);
> + rcar_dab_write(dab, RCAR_DAB_FFTNUMOFDMR, fft->ofdm_number);
> + rcar_dab_write(dab, RCAR_DAB_FFTINADDR, (u32)dab->fft.dma_input_buf);
> + rcar_dab_write(dab, RCAR_DAB_FFTOUTADDR, (u32)dab->fft.dma_output_buf);
Maybe use lower_32_bits() instead of the (u32) cast.
For clarity, you may also want to specifically ask for a 32-bit DMA mask
in the probe function, with a comment that describes what the hardware
limitation is.
> +
> + if (copy_from_user(dab->fft.input_buffer, fft_req->input_address,
> + buffer_size)) {
> + mutex_unlock(&dab->fft.lock);
> + return -EFAULT;
> + }
> +
> + dab->fft.done = false;
> + ret = rcar_dab_fft_init(dab, fft_req);
> + if (ret) {
> + mutex_unlock(&dab->fft.lock);
> + return ret;
> + }
> +
> + rcar_dab_fft_enable(dab);
> + wait_event_interruptible_timeout(dab->fft.wait, dab->fft.done, HZ);
> + if (!dab->fft.done) {
> + rcar_dab_fft_disable(dab);
> + ret = -EFAULT;
-EFAULT doesn't look like the right error for timeout or signal
handling. Better check the return code from wait_event_interruptible_timeout()
instead.
> +
> +struct rcar_dab_fft_req {
> + int points; /*
> + * The number of points to use.
> + * Legal values are 256, 512, 1024, and
> + * 2048.
> + */
> + unsigned char ofdm_number; /*
> + * Orthogonal Frequency Division
> + * Multiplexing (OFDM).
> + * Minimum value is 1, maximum value is
> + * 255.
> + */
> + void __user *input_address; /*
> + * User space address for the input
> + * buffer.
> + */
> + void __user *output_address; /*
> + * User space address for the output
> + * buffer.
> + */
> +};
Please read Documentation/driver-api/ioctl.rst and make this a portable
data structure.
Arnd
Powered by blists - more mailing lists