linux-kernel - Re: [PATCH 3/3] SPI: Ingenic: Add SFC support for Ingenic SoCs.

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <89d22457-8c62-e441-3bf4-2734ec2a45e1@wanyeetech.com>
Date:   Sun, 24 Jul 2022 01:06:16 +0800
From:   Zhou Yanjie <zhouyanjie@...yeetech.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     tudor.ambarus@...rochip.com, p.yadav@...com, michael@...le.cc,
        miquel.raynal@...tlin.com, richard@....at, vigneshr@...com,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        linux-mtd@...ts.infradead.org, linux-spi@...r.kernel.org,
        linux-mips@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, aidanmacdonald.0x0@...il.com,
        tmn505@...il.com, paul@...pouillou.net, dongsheng.qiu@...enic.com,
        aric.pzqi@...enic.com, rick.tyliu@...enic.com,
        jinghui.liu@...enic.com, sernia.zhou@...mail.com,
        reimu@...omaker.com
Subject: Re: [PATCH 3/3] SPI: Ingenic: Add SFC support for Ingenic SoCs.

Hi Mark,

On 2022/7/23 上午2:38, Mark Brown wrote:
> On Sat, Jul 23, 2022 at 12:48:30AM +0800, 周琰杰 (Zhou Yanjie) wrote:
>
> This looks mostly good, a few small issues though:
>
>> +++ b/drivers/spi/spi-ingenic-sfc.c
>> @@ -0,0 +1,662 @@
>> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +/*
>> + * Ingenic SoCs SPI Flash Controller Driver
> Please make the entire comment a C++ one so things look more
> intentional.


I'm sorry, I didn't understand well what you meant :(
Could you please explain a little more detail?


>
>> +static irqreturn_t ingenic_sfc_irq_handler(int irq, void *data)
>> +{
>> +	struct ingenic_sfc *sfc = data;
>> +
>> +	writel(0x1f, sfc->base + SFC_REG_INTC);
>> +
>> +	complete(&sfc->completion);
>> +
>> +	return IRQ_HANDLED;
>> +}
> This doesn't pay any attention to any status registers in the chip so
> won't work if the interrupt is shared and won't notice any error reports
> from the device...


This interrupt is exclusively owned by SFC, do we still
need to perform the operation you said? I haven't done
these operations before because I want to minimize the
overhead and avoid affecting performance.


>
>> +static int ingenic_sfc_setup(struct spi_device *spi)
>> +{
>> +	struct ingenic_sfc *sfc = spi_controller_get_devdata(spi->master);
>> +	unsigned long rate;
>> +	int ret, val;
>> +
>> +	if (!spi->max_speed_hz)
>> +		return -EINVAL;
>> +
>> +	ret = clk_set_rate(sfc->clk, spi->max_speed_hz * 2);
>> +	if (ret)
>> +		return -EINVAL;
> The setup() operation should be safe for use on one device while another
> device is active.  It's not going to be a problem until there's a
> version of the IP with more than one chip select, but that could happen
> some time (and someone might decide to make a board using GPIO chip
> selects...) but this should really go into the data path.


Sure, I will change it in the next version.


>> +	ret = clk_prepare_enable(sfc->clk);
>> +	if (ret)
>> +		goto err_put_master;
> Nothing ever disables this clock.  It might also be nice to enable the
> clock only when the controller is in use, that bit is not super
> important though.


Sure, will add it.


>
>> +	ret = devm_request_irq(&pdev->dev, sfc->irq, ingenic_sfc_irq_handler, 0,
>> +			dev_name(&pdev->dev), sfc);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Failed to request irq%d, ret = %d\n", sfc->irq, ret);
>> +		goto err_put_master;
>> +	}
> It's not safe to use devm here...


Sure, will fix it in the next version.


>
>> +	ret = devm_spi_register_controller(&pdev->dev, ctlr);
>> +	if (ret)
>> +		goto err_put_master;
> ...unregistering the controller may free the driver data structure and
> the interrupt handler uses it so we could attempt to use freed data in
> the window between the controller being unregistered and the interrupt
> being freed.


Sure.


Thanks and best regards!