| 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: <2493fc3fa3f6e5d2bcdde27cee1c33df@codeaurora.org>
Date: Fri, 07 Sep 2018 15:30:44 +0530
From: dkota@...eaurora.org
To: Doug Anderson <dianders@...omium.org>
Cc: Stephen Boyd <swboyd@...omium.org>,
Mark Brown <broonie@...nel.org>,
Matthias Kaehlcke <mka@...omium.org>,
LKML <linux-kernel@...r.kernel.org>,
linux-spi <linux-spi@...r.kernel.org>,
Andy Gross <andy.gross@...aro.org>,
David Brown <david.brown@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
linux-arm-msm <linux-arm-msm@...r.kernel.org>,
"open list:ARM/QUALCOMM SUPPORT" <linux-soc@...r.kernel.org>,
devicetree@...r.kernel.org,
Girish Mahadevan <girishm@...eaurora.org>
Subject: Re: [PATCH V3] spi: spi-geni-qcom: Add SPI driver support for GENI
based QUP
On 2018-09-02 10:36, Doug Anderson wrote:
> Hi,
>
> On Fri, Aug 24, 2018 at 3:42 AM, Dilip Kota <dkota@...eaurora.org>
> wrote:
>> From: Girish Mahadevan <girishm@...eaurora.org>
>>
>> This driver supports GENI based SPI Controller in the Qualcomm SOCs.
>> The
>> Qualcomm Generic Interface (GENI) is a programmable module supporting
>> a
>> wide range of serial interfaces including SPI. This driver supports
>> SPI
>> operations using FIFO mode of transfer.
>>
>> Signed-off-by: Girish Mahadevan <girishm@...eaurora.org>
>> Signed-off-by: Dilip Kota <dkota@...eaurora.org>
>> ---
>> Addressing all the reviewer commets given in Patchset1.
>> Summerizing all the comments below:
>>
>> MAKEFILE: Arrange SPI-GENI driver in alphabetical order
>> Kconfig: Mark SPI_GENI driver dependent on QCOM_GENI_SE
>> Enable SPI core auto runtime pm, and remove runtime pm calls.
>> Remove spi_geni_unprepare_message(),
>> spi_geni_unprepare_transfer_hardware()
>> Remove likely/unlikely keywords.
>> Remove get_spi_master() and use dev_get_drvdata()
>> Move request_irq to probe()
>> Mark bus number assignment to -1 as SPI core framework will
>> assign dynamically
>> Use devm_spi_register_master()
>> Include platform_device.h instead of of_platform.h
>> Removing macros which are used only once:
>> #define SPI_NUM_CHIPSELECT 4
>> #define SPI_XFER_TIMEOUT_MS 250
>> Place Register field definitions next to respective Register
>> definitions.
>> Replace int and u32 declerations to unsigned int.
>> Remove Hex numbers in debug prints.
>> Declare mode as u16 in spi_setup_word_len()
>> Remove the labels: setup_fifo_params_exit:
>> exit_prepare_transfer_hardware:
>> Declaring struct spi_master as spi everywhere in the file.
>> Calling spi_finalize_current_transfer() for end of transfer.
>> Hard code the SPI controller max frequency instead of reading
>> from DTSI node.
>> Spinlock not required, removed it.
>> Removed unrequired error prints.
>> Fix KASAN error in geni_spi_isr().
>> Remove spi-geni-qcom.h
>> Remove inter words delay and CS to Clock toggle delay logic in
>> the driver, as of now no clients are using it.
>> Will submit this logic in the next patchset.
>> Use major, minor and step macros to read from hardware version
>> register.
>>
>> .../devicetree/bindings/soc/qcom/qcom,geni-se.txt | 2 -
>> drivers/spi/Kconfig | 12 +
>> drivers/spi/Makefile | 1 +
>> drivers/spi/spi-geni-qcom.c | 678
>> +++++++++++++++++++++
>> 4 files changed, 691 insertions(+), 2 deletions(-)
>
> See below for comments. In general I've tried to post patches to
> address my own comments. See the series ending at
> <https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201142/1>.
> From there you can download patch files by using the "DOWNLOAD" link
> at the bottom. Yell if you have problems. Hopefully that's useful.
> I expect that you can squash many of these into your patch to give you
> a leg up on v3.
>
> NOTE: I won't promise that I made no mistakes on my fixup patches nor
> that I caught everything or did everything right. I'll plan to take a
> fresh look at the whole patch when I see your v3.
>
>
>> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>> @@ -60,7 +60,6 @@ Required properties:
>> - interrupts: Must contain SPI controller interrupts.
>> - clock-names: Must contain "se".
>> - clocks: Serial engine core clock needed by the device.
>> -- spi-max-frequency: Specifies maximum SPI clock frequency, units -
>> Hz.
>
> As per Rob's feedback, please split the device tree change into a
> separate patch and justify it. Perhaps the commit message could be
> something like:
>
> ---
Ok, will submit as separate patch.
>
> dt-bindings: spi: Remove spi-max-frequency from qcom,geni-se controller
>
> No other SPI controllers have a "spi-max-frequency" at the controller
> level. The normal "spi-max-frequency" property is something that is
> used when defining the nodes for SPI slaves. While it is possible
> that someone might want to define a controller-level max frequency it
> should be done in other ways (perhaps by keying off a compatible
> string?)
>
> ---
>
> I think in the past Mark Brown has also requested that the bindings
> actually live under "Documentation/devicetree/bindings/spi/", so
> perhaps you should also add a patch to your series that moves this
> documentation there and changes the "soc/qcom/qcom,geni-se.txt" to
> reference that.
>
>
>> +static irqreturn_t geni_spi_isr(int irq, void *data);
>> +
>> +struct spi_geni_master {
>> + struct geni_se se;
>> + unsigned int irq;
>
> In v1 Stephen requested that many things in this struct become
> "unsigned int", but he didn't mean the "irq". Please change this back
> to an int. As you have things right now the code "if (spi_geni->irq <
> 0)" you have below is a no-op. :(
>
> ...oh, and as Stephen pointed out to me offline you don't even need to
> store irq.
>
Ok
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201065
>
>
>> + struct device *dev;
>> + unsigned int rx_fifo_depth;
>> + unsigned int tx_fifo_depth;
>> + unsigned int tx_fifo_width;
>> + unsigned int tx_wm;
>
> Anything that is effectively the contents of a hardware register
> should be of type 'u32'.
Ok, will update it
>
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201066
>
>
>> +static int get_spi_clk_cfg(unsigned int speed_hz,
>> + struct spi_geni_master *mas,
>> + unsigned int *clk_idx,
>> + unsigned int *clk_div)
>> +{
>> + unsigned long sclk_freq;
>> + struct geni_se *se = &mas->se;
>> + int ret;
>> +
>> + ret = geni_se_clk_freq_match(&mas->se,
>> + (speed_hz * mas->oversampling),
>> clk_idx,
>> + &sclk_freq, true);
>
> Pass "false" as the last parameter after picking my geni patch to make
> this work right. For my geni patch see
> <https://lkml.kernel.org/r/20180830183612.169572-2-dianders@chromium.org>.
>
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201064
> Agree. Will mark it to False.
>
>> + if (ret) {
>> + dev_err(mas->dev, "%s: Failed(%d) to find src clk for
>> %dHz\n",
>> + __func__, ret,
>> speed_hz);
>
> As I said in previous reviews: printing __func__ is generally
> discouraged for dev_xx printouts. They've already got the device name
> and that together with the string should be enough. Remove it here
> and elsewhere in this file.
>
Ok, will traverse the driver and remove __func__
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201067
>
>
>> + return ret;
>> + }
>> +
>> + *clk_div = ((sclk_freq / mas->oversampling) / speed_hz);
>
> After changing exact to false, you'll want:
>
> *clk_div = DIV_ROUND_UP(sclk_freq, mas->oversampling * speed_hz);
> actual_hz = sclk_freq / (mas->oversampling * *clk_div);
>
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201064
>
Ok. I see actual_hz variable for debug prints itself.
>
>> + if (!(*clk_div)) {
>> + dev_err(mas->dev, "%s:Err:sclk:%lu oversampling:%d
>> speed:%u\n",
>> + __func__, sclk_freq, mas->oversampling,
>> speed_hz);
>> + return -EINVAL;
>> + }
>
> After change above to use DIV_ROUND_UP() you no longer need to check
> for a zero clk_div.
Ok
>
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201064
>
>
>> +static void spi_setup_word_len(struct spi_geni_master *mas, u16 mode,
>> + unsigned int bits_per_word)
>> +{
>> + unsigned int word_len, pack_words;
>> + bool msb_first = (mode & SPI_LSB_FIRST) ? false : true;
>> + struct geni_se *se = &mas->se;
>> +
>> + word_len = readl_relaxed(se->base + SE_SPI_WORD_LEN);
>
> Gratuitously using the __relaxed versions of readl() and writel() is
> discouraged in Linux because it gives very little performance gain in
> 99.9% of the cases and it makes the code significantly harder to
> reason about it. Remove it in spi-geni-qcom. If it is shown that a
> particular read or write is in a performance critical section then
> that one instance can be added back in after carefully analysis and
> documentation.
>
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201068
Ok
>
>
>> +static int setup_fifo_params(struct spi_device *spi_slv,
>> + struct spi_master *spi)
>> +{
>> + struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> + struct geni_se *se = &mas->se;
>> + unsigned int mode = spi_slv->mode;
>> + unsigned int loopback_cfg = readl_relaxed(se->base +
>> SE_SPI_LOOPBACK);
>> + unsigned int cpol = readl_relaxed(se->base + SE_SPI_CPOL);
>> + unsigned int cpha = readl_relaxed(se->base + SE_SPI_CPHA);
>> + unsigned int demux_sel, clk_sel, m_clk_cfg, idx, div;
>> + int ret = 0;
>
> In v1 Stephen said:
>
>> Don't initialize variables and then overwrite them.
>
> This is in lots of places in your code. I've found and fixed them for
> you:
>
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201070
Ok
>
>
>> + unsigned int demux_output_inv = 0;
>> +
>> + loopback_cfg &= ~LOOPBACK_MSK;
>> + cpol &= ~CPOL;
>> + cpha &= ~CPHA;
>> +
>> + if (mode & SPI_LOOP)
>> + loopback_cfg |= LOOPBACK_ENABLE;
>> +
>> + if (mode & SPI_CPOL)
>> + cpol |= CPOL;
>> +
>> + if (mode & SPI_CPHA)
>> + cpha |= CPHA;
>> +
>> + if (spi_slv->mode & SPI_CS_HIGH)
>> + demux_output_inv = BIT(spi_slv->chip_select);
>> +
>> + demux_sel = spi_slv->chip_select;
>> + mas->cur_speed_hz = spi_slv->max_speed_hz;
>> + mas->cur_word_len = spi_slv->bits_per_word;
>> +
>> + ret = get_spi_clk_cfg(mas->cur_speed_hz, mas, &idx, &div);
>> + if (ret) {
>> + dev_err(mas->dev, "Err setting clks ret(%d) for %d\n",
>> + ret,
>> mas->cur_speed_hz);
>> + return ret;
>> + }
>> +
>> + clk_sel = (idx & CLK_SEL_MSK);
>> + m_clk_cfg = ((div << CLK_DIV_SHFT) | SER_CLK_EN);
>
> Please avoid useless parenthesis everywhere in this file. Stephen
> pointed out many of these in v1 but the correct thing to do would have
> been to search for all instances and fix them, not just fix the ones
> that Stephen pointed out. I've now done it for you:
>
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201071
>
>
Ok
>> +static int spi_geni_prepare_transfer_hardware(struct spi_master *spi)
>> +{
>> + struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> + struct geni_se *se = &mas->se;
>> +
>> + if (!mas->setup) {
>> + unsigned int proto = geni_se_read_proto(se);
>> + unsigned int major, minor, step, ver;
>> +
>> + if (proto != GENI_SE_SPI) {
>> + dev_err(mas->dev, "Invalid proto %d\n",
>> proto);
>> + return -ENXIO;
>> + }
>> + mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se);
>> + mas->rx_fifo_depth = geni_se_get_rx_fifo_depth(se);
>> + mas->tx_fifo_width = geni_se_get_tx_fifo_width(se);
>> +
>> + /*
>> + * Hardware programming guide suggests to configure
>> + * RX FIFO RFR level to fifo_depth-2.
>> + */
>> + geni_se_init(se, 0x0, mas->tx_fifo_depth - 2);
>> + mas->oversampling = 1;
>> + /* Transmit an entire FIFO worth of data per IRQ */
>> + mas->tx_wm = 1;
>> + ver = geni_se_get_qup_hw_version(se);
>> + major = GENI_SE_VERSION_MAJOR(ver);
>> + minor = GENI_SE_VERSION_MINOR(ver);
>> + step = GENI_SE_VERSION_STEP(ver);
>
> You don't use "step". Kill it.
>
Ok
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201069
>
>
>> + /*
>> + * If CS change flag is set, then toggle the CS line in
>> between
>> + * transfers and keep CS asserted after the last transfer.
>> + * Else if keep CS flag asserted in between transfers and
>> de-assert
>> + * CS after the last message.
>> + */
>> + if (xfer->cs_change) {
>> + if (list_is_last(&xfer->transfer_list,
>> + &spi->cur_msg->transfers))
>> + m_param |= FRAGMENTATION;
>> + } else {
>> + if (!list_is_last(&xfer->transfer_list,
>> + &spi->cur_msg->transfers))
>
> In v1 Stephen said:
>
>> Combine else and if here into an else if.
>
> ...but really I think you could just do:
>
> /*
> * Keep the CS asserted in either of the two cases:
> * - It's not the last transfer (and cs_change is not set)
> * - It is the last transfer (and cs_change is set)
> *
> * ...AKA keep it asserted whenever the "last transfer" and cs_change
> * booleans agree.
> */
> if (!!xfer->cs_change ==
> !!list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers))
> m_param |= FRAGMENTATION;
>
This complete logic will be removed with set_cs() change, because
m_param |= FRAGMENTATION; should set in all the cases.
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201072
>
>
>> + if (m_cmd & SPI_TX_ONLY) {
>> + mas->tx_rem_bytes = xfer->len;
>> + writel_relaxed(trans_len, se->base +
>> SE_SPI_TX_TRANS_LEN);
>> + }
>> +
>> + if (m_cmd & SPI_RX_ONLY) {
>> + writel_relaxed(trans_len, se->base +
>> SE_SPI_RX_TRANS_LEN);
>> + mas->rx_rem_bytes = xfer->len;
>> + }
>> + writel_relaxed(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
>> + geni_se_setup_m_cmd(se, m_cmd, m_param);
>> + if (m_cmd & SPI_TX_ONLY)
>> + writel_relaxed(mas->tx_wm, se->base +
>> SE_GENI_TX_WATERMARK_REG);
>
> In v1 Stephen said:
>
>> This can't be combined with above m_cmd & SPI_TX_ONLY statement?
>
> In v2 I said:
>
>> Girish said it can't. ...but IMO if it really can't then please add a
>> comment in the code so someone doesn't later go back and try to
>> combine.
>
> I don't see a comment. An example of an appropriate comment (assuming
> that the geni hardware really does need the services of a dark wizard)
> would be:
>
I will add the detailed comments.
> /*
> * NOTE: we have to write the SE_GENI_TX_WATERMARK_REG _after_ the
> * write to SE_SPI_TRANS_CFG and the call to geni_se_setup_m_cmd()
> * because these two things perform a special incantation to summon a
> * dark wizard and we're not allowed to set the watermark until the
> * dark wizard arrives.
> */
>
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201073
>
> Presumably writing to the watermark kicks off the interrupt happening
> so it has to be last. That would be something to say instead of
> talking about magic.
>
>
>> +static irqreturn_t geni_spi_handle_tx(struct spi_geni_master *mas)
>> +{
>> + unsigned int i = 0;
>> + unsigned int tx_fifo_width = mas->tx_fifo_width /
>> BITS_PER_BYTE;
>
> This is super confusing. You have two things with the same name
> "tx_fifo_width" one that's in terms of bits and one that's in terms of
> bytes. ...in general it's really hard to figure out the units of all
> your variables here. For things that it's not clear you should rename
> variables.
>
Ok, will rename it.
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201075
>
>
>> + unsigned int max_bytes;
>> + const u8 *tx_buf;
>> + struct geni_se *se = &mas->se;
>> +
>> + if (!mas->cur_xfer)
>> + return IRQ_NONE;
>> +
>> + /*
>> + * For non-byte aligned bits-per-word values(e.g 9).
>> + * The FIFO packing is set to 1 SPI word per FIFO word.
>
> In v1 Stephen said:
>
>> Decapitalize "The"
>
> You added a "." at the end of the first line so I guess it's not quite
> as wrong as it was but it's still better grammar to just have one
> sentence.
>
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201074
>
>
Ok
>> + * Assumption is that each SPI word will be accomodated in
>> + * ceil (bits_per_word / bits_per_byte)
>> + * and the next SPI word starts at the next byte.
>> + * In such cases, we can fit 1 SPI word per FIFO word so
>> adjust the
>> + * max byte that can be sent per IRQ accordingly.
>> + */
>> + max_bytes = (mas->tx_fifo_depth - mas->tx_wm);
>> + if (mas->tx_fifo_width % mas->cur_word_len)
>> + max_bytes *= ((mas->cur_word_len / BITS_PER_BYTE) +
>> 1);
>
> Even though the math should be the same (since you should you'll never
> divide cur_word_len evenly), the above is clearer as:
>
> max_bytes *= DIV_ROUND_UP(mas->cur_word_len, BITS_PER_BYTE);
>
> ...oh, but even that is wrong. SPI framework documents that we need
> the nearest power of 2, so a 20-bit transfer will not take up 3 bytes
> but will actually take up 4.
>
> ...and of course this same assumption is replicated twice in both the
> tx and rx functions. I'm just going to rewrite these two functions.
>
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201076
>
>
>> + while (i < max_bytes) {
>> + unsigned int j;
>> + unsigned int fifo_word = 0;
>> + u8 *fifo_byte;
>> + unsigned int bytes_per_fifo = tx_fifo_width;
>> + unsigned int bytes_to_write = 0;
>> +
>> + if (mas->tx_fifo_width % mas->cur_word_len)
>> + bytes_per_fifo =
>> + (mas->cur_word_len / BITS_PER_BYTE) +
>> 1;
>> +
>> + if (bytes_per_fifo < (max_bytes - i))
>> + bytes_to_write = bytes_per_fifo;
>> + else
>> + bytes_to_write = max_bytes - i;
>> +
>> + fifo_byte = (u8 *)&fifo_word;
>> + for (j = 0; j < bytes_to_write; j++)
>> + fifo_byte[j] = tx_buf[i++];
>
> In previous reviews Stephen pointed out that he's not super keen on
> this manual copy of bytes, but IMO it's not a huge deal and we can
> find a better way to do it later. ...but if someone wants to propose
> a patch that makes it nicer then I won't object.
>
>
>> +static irqreturn_t geni_spi_handle_rx(struct spi_geni_master *mas)
>> +{
>> + unsigned int i = 0;
>> + struct geni_se *se = &mas->se;
>> + unsigned int fifo_width = mas->tx_fifo_width / BITS_PER_BYTE;
>
> You're assuming TX and RX FIFO widths are the same. Let's make that
> explicit.
>
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201075
>
Agree
>
>> +static int spi_geni_probe(struct platform_device *pdev)
>> +{
>> + int ret;
>> + struct spi_master *spi;
>> + struct spi_geni_master *spi_geni;
>> + struct resource *res;
>> + struct geni_se *se;
>> +
>> + spi = spi_alloc_master(&pdev->dev, sizeof(struct
>> spi_geni_master));
>> + if (!spi) {
>> + ret = -ENOMEM;
>> + goto spi_geni_probe_err;
>
> When spi_alloc_master() fails you don't want spi_master_put().
>
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201078
>
>
>> + spi->prepare_transfer_hardware =
>> spi_geni_prepare_transfer_hardware;
>
> IMO this is wrong. You're using prepare_transfer_hardware() to do
> one-time init. Just do it at probe.
>
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201079
>
Agree, will move it to probe.
>
>> + init_completion(&spi_geni->xfer_done);
>> + pm_runtime_enable(&pdev->dev);
>> + ret = devm_spi_register_master(&pdev->dev, spi);
>> + if (ret)
>> + goto spi_geni_probe_unmap;
>
> In this error case I believe you need pm_runtime_disable().
>
Yes, I will update it.
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201078
>
>
>> + return ret;
>> +spi_geni_probe_unmap:
>> + devm_iounmap(&pdev->dev, se->base);
>
> The whole point of devm is that you don't need this kind of the error
> handling.
>
Agree
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201078
>
>
>
>> +static int spi_geni_remove(struct platform_device *pdev)
>> +{
>> + struct spi_master *spi = platform_get_drvdata(pdev);
>> + struct spi_geni_master *spi_geni =
>> spi_master_get_devdata(spi);
>> +
>> + geni_se_resources_off(&spi_geni->se);
>
> In v2 I said:
>
>> Why would you need to call geni_se_resources_off()? Isn't it handled
>> by pm_runtime?
IMO, runtime_pm may not be called during unbind operation. Anyhow I will
run the test and check it.
>
>
>> + pm_runtime_put_noidle(&pdev->dev);
>
> In v2 I said:
>
>> I'm curious: why would you have a "put" here? Won't that result in an
>> imbalance since you don't exit probe with "get"? ...or did pm_runtime
>> throw me for a loop again?
>
>> + pm_runtime_disable(&pdev->dev);
>> + return 0;
>
> So testing:
>
> cd /sys/bus/platform/drivers/geni_spi/
> echo 880000.spi > unbind
> sleep 2
> echo 880000.spi > bind
>
> ...in fact it yells a lot. To me this means that you didn't test
> spi_geni_remove() at all.
>
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201077
>
I will test and update it.
>
>> +static int __maybe_unused spi_geni_suspend(struct device *dev)
>> +{
>> + if (!pm_runtime_status_suspended(dev))
>> + return -EBUSY;
>
> I didn't have time to dig into Stephen Boyd's feedback, but please
> make sure you address it here. I agree that something seems fishy
> here.
>
I will check it.
> ---
>
> In v2, I said:
>
>> I'm not sure where to comment about this, so adding it to the end:
>>
>> Between v1 and v2 you totally removed all the locking. Presumably
>> this is because you didn't want to hold the lock in
>> handle_fifo_timeout() while waiting for the completion. IMO taking
>> the lock out was the wrong thing to do. You should keep it, but just
>> drop the lock before wait_for_completion_timeout() and add it back
>> afterwards. Specifically you _don't_ want the IRQ and timeout code
>> stomping on each other.
>
> ...but still no spinlock?
I see there is no need of taking the spinlock as timeout will be handled
after the calculated time as per data size and speed.
There is 99.9% less chances of interrupt during the timeout handler.
>
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201081
>
> ---
>
> As we've discussed offline, your current driver doesn't deal with chip
> select in a correct enough way to make cros_ec_spi work. I've applied
> and fixed up your WIP patch to fix this.
You are correct, runtime_pm wont be called by spi framework during the
setup call. And, in ISR flag is required to differentiate between set_cs
and actual transfer.
>
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201142
>
>
> -Doug
Powered by blists - more mailing lists