linux-kernel - Re: [EXT] Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx series chip

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: <cbe2baaf-5529-a9c8-b1d1-b22ef294a1e7@nxp.com>
Date:   Wed, 30 Nov 2022 09:25:26 +0530
From:   Manjunatha Venkatesh <manjunatha.venkatesh@....com>
To:     Arnd Bergmann <arnd@...db.de>, linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Will Deacon <will@...nel.org>, Jens Axboe <axboe@...nel.dk>,
        Rob Herring <robh+dt@...nel.org>
Cc:     mb@...htnvm.io, ckeepax@...nsource.cirrus.com, arnd@...db.d,
        "Michael S. Tsirkin" <mst@...hat.com>, javier@...igon.com,
        "Michael Kelley (LINUX)" <mikelley@...rosoft.com>,
        Jason Wang <jasowang@...hat.com>, sunilmut@...rosoft.com,
        bjorn.andersson@...aro.org, krzysztof.kozlowski+dt@...aro.org,
        devicetree@...r.kernel.org, ashish.deshpande@....com,
        rvmanjumce@...il.com
Subject: Re: [EXT] Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for
 sr1xx series chip


On 10/7/2022 7:41 PM, Arnd Bergmann wrote:
> Caution: EXT Email
>
> On Fri, Oct 7, 2022, at 4:04 PM, Manjunatha Venkatesh wrote:
>> sr1xx_dev_open(struct inode *inode, struct file *filp)
>>>> +{
>>>> +     struct sr1xx_dev *sr1xx_dev =
>>>> +         container_of(filp->private_data, struct sr1xx_dev, sr1xx_device);
>>>> +
>>>> +     filp->private_data = sr1xx_dev;
>>> This looks dangerous if the file gets opened more than once
>>> and filp->private_data can have two different values.
>> Do you suggest us to use mutex lock inside open api?
>
>>>> +
>>>> +     sr1xx_dev->spi = spi;
>>>> +     sr1xx_dev->sr1xx_device.minor = MISC_DYNAMIC_MINOR;
>>>> +     sr1xx_dev->sr1xx_device.name = "sr1xx";
>>>> +     sr1xx_dev->sr1xx_device.fops = &sr1xx_dev_fops;
>>>> +     sr1xx_dev->sr1xx_device.parent = &spi->dev;
>>>> +     sr1xx_dev->irq_gpio = desc_to_gpio(platform_data.gpiod_irq);
>>>> +     sr1xx_dev->ce_gpio = desc_to_gpio(platform_data.gpiod_ce);
>>>> +     sr1xx_dev->spi_handshake_gpio =
>>>> +         desc_to_gpio(platform_data.gpiod_spi_handshake);
>>> The temporary 'platform_data' structure seems useless here,
>>> just fold its members into the sr1xx_dev structure itself.
>>> No need to store both a gpio descriptor and a number, you
>>> can simplify this to always use the descriptor.
>> Just to keep separate function(sr1xx_hw_setup) for better readability
>> we have added intermediate platform_data structure.
> I'm fairly sure it adds nothing to readability if every reader has
> to wonder about why you have a platform_data structure here when
> the device was never used without devicetree.
Will fix this in the next patch submission.
>>>> +     sr1xx_dev->tx_buffer = kzalloc(SR1XX_TXBUF_SIZE, GFP_KERNEL);
>>>> +     sr1xx_dev->rx_buffer = kzalloc(SR1XX_RXBUF_SIZE, GFP_KERNEL);
>>>> +     if (!sr1xx_dev->tx_buffer) {
>>>> +             ret = -ENOMEM;
>>>> +             goto err_exit;
>>>> +     }
>>>> +     if (!sr1xx_dev->rx_buffer) {
>>>> +             ret = -ENOMEM;
>>>> +             goto err_exit;
>>>> +     }
>>>> +
>>>> +     sr1xx_dev->spi->irq = gpio_to_irq(sr1xx_dev->irq_gpio);
>>>> +     if (sr1xx_dev->spi->irq < 0) {
>>>> +             dev_err(&spi->dev, "gpio_to_irq request failed gpio = 0x%x\n",
>>>> +                     sr1xx_dev->irq_gpio);
>>>> +             goto err_exit;
>>>> +     }
>>> Instead of gpio_to_irq(), the DT binding should probably
>>> list the interrupt directly using the "interrupts" property
>>> pointing to the gpio controller. Since, we are configured this as generic GPIO in DT binding. So, we
>> are using gpio_to_irq() to use as IRQ pin.
> I meant you should change the binding first, and then adapt the
> code to match. Remove all references to gpio numbers from
> the code.
Will work on this suggestion and update in the next patch submission.
>      Arnd