| 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: <dfba682c-c855-df9c-4081-cd65ada5f61b@broadcom.com>
Date: Thu, 26 Jan 2023 18:59:48 -0800
From: William Zhang <william.zhang@...adcom.com>
To: Jonas Gorski <jonas.gorski@...il.com>
Cc: Linux SPI List <linux-spi@...r.kernel.org>,
Broadcom Kernel List <bcm-kernel-feedback-list@...adcom.com>,
tomer.yacoby@...adcom.com, kursad.oney@...adcom.com,
dregan@...l.com, f.fainelli@...il.com, anand.gore@...adcom.com,
dan.beygelman@...adcom.com, joel.peshkin@...adcom.com,
kernel test robot <lkp@...el.com>,
Mark Brown <broonie@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 10/14] spi: bcm63xx-hsspi: Add prepend mode support
On 01/26/2023 07:15 AM, Jonas Gorski wrote:
>> +static int bcm63xx_hsspi_wait_cmd(struct bcm63xx_hsspi *bs)
>> +{
>> + unsigned long limit;
>> + u32 reg = 0;
>> + int rc = 0;
>
> If the only possible return values are 0 and 1, maybe this should be a bool?
>
Well it is possible we may want to return to some specific error code so
I would prefer to keep it as is.
>> +
>> + if (bs->wait_mode == HSSPI_WAIT_MODE_INTR) {
>> + if (wait_for_completion_timeout(&bs->done, HZ) == 0)
>> + rc = 1;
>> + } else {
>> + /* polling mode checks for status busy bit */
>> + limit = jiffies + msecs_to_jiffies(HSSPI_POLL_STATUS_TIMEOUT_MS);
>> +
>> + while (!time_after(jiffies, limit)) {
>> + reg = __raw_readl(bs->regs + HSSPI_PINGPONG_STATUS_REG(0));
>> + if (reg & HSSPI_PINGPONG_STATUS_SRC_BUSY)
>> + cpu_relax();
>> + else
>> + break;
>> + }
>> + if (reg & HSSPI_PINGPONG_STATUS_SRC_BUSY)
>> + rc = 1;
>> + }
>> +
>> + if (rc)
>> + dev_err(&bs->pdev->dev, "transfer timed out!\n");
>> +
>> + return rc;
>> +}
>> +
>> +static bool bcm63xx_check_msg_prependable(struct spi_master *master,
>> + struct spi_message *msg,
>> + struct spi_transfer *t_prepend)
>
> This function does more than just checking, so I think a more
> appropriate name would be something like
>
> bcm63xx_prepare_prepend_transfer()
>
That's reasonable. The function kind of evolved from the checking only.
>> +{
>> +
>> + struct bcm63xx_hsspi *bs = spi_master_get_devdata(master);
>> + bool prepend = false, tx_only = false;
>> + struct spi_transfer *t;
>> +
>> + /* If it is forced cs dummy workaround mode, no need to prepend message */
>> + if (bs->xfer_mode == HSSPI_XFER_MODE_DUMMYCS)
>> + return false;
>
> That's a weird point for that, why not just move this to the caller
> and check it before calling the function.
>
True following your above function name change suggestion.
>> +
>> + /*
>> + * Multiple transfers within a message may be combined into one transfer
>> + * to the controller using its prepend feature. A SPI message is prependable
>> + * only if the following are all true:
>> + * 1. One or more half duplex write transfer in single bit mode
>> + * 2. Optional full duplex read/write at the end
>> + * 3. No delay and cs_change between transfers
>> + */
>> + bs->prepend_cnt = 0;
>> + list_for_each_entry(t, &msg->transfers, transfer_list) {
>> + if ((spi_delay_to_ns(&t->delay, t) > 0) || t->cs_change) {
>> + dev_warn(&bs->pdev->dev,
>> + "Delay or cs change not supported in prepend mode!\n");
>
> I don't think warn is the right level. If we are on XFER_MODE_AUTO,
> this should be _dbg, since we will just fall back to the dummy cs
> mode, if we are on XFER_MODE_PREPEND, this should be dev_err, since we
> cannot do the message.
>
I was relying on this to see the message when we fall back. But
certainly I can fine tune the message level as you suggested
> cs->change is technically supported when all that's requested is a
> between transfer cs toggle (t->cs_change is true, t->cs_off is false
> and next transfer's cs_off is also false), which automatically happens
> after the transfer. Not sure if it is worth the effort implementing
> that though.
>
If this cs toggling is between the transfers that we are merging here,
then no as the cs will be always active during merged transfer in
prepend mode.
>> + break;
>> + }
>> +
>> + tx_only = false;
>> + if (t->tx_buf && !t->rx_buf) {
>> + tx_only = true;
>> + if (bs->prepend_cnt + t->len >
>> + (HSSPI_BUFFER_LEN - HSSPI_OPCODE_LEN)) {
>> + dev_warn(&bs->pdev->dev,
>> + "exceed max buf len, abort prepending transfers!\n");
>> + break;
>
> why not just return false here directly? And everywhere else where you
> decided that you cannot use prepend.
> Sure I can eliminate the prepend variable and return directly
>> + }
>> +
>> + if (t->tx_nbits > SPI_NBITS_SINGLE &&
>> + !list_is_last(&t->transfer_list, &msg->transfers)) {
>> + dev_warn(&bs->pdev->dev,
>> + "multi-bit prepend buf not supported!\n");
>> + break;
>> + }
>> +
>> + if (t->tx_nbits == SPI_NBITS_SINGLE) {
>> + memcpy(bs->prepend_buf + bs->prepend_cnt, t->tx_buf, t->len);
>> + bs->prepend_cnt += t->len;
>> + }
>> + } else {
>> + if (!list_is_last(&t->transfer_list, &msg->transfers)) {
>> + dev_warn(&bs->pdev->dev,
>> + "rx/tx_rx transfer not supported when it is not last one!\n");
>
> This is only an issue if doing multi-bit RX/TX; for single bit you can
> just upgrade the whole transfer/message to duplex, you just need to
> pick the read bytes then from the right offsets.
>
I am not sure if this will work all the case. Considering two transfers
rx 3 bytes then tx 3 bytes in the message(not sure if there is any
device requires this kind of message but just for discussion
purpose...), if I upgrade it to duplex message, the controller will
transfer and receive 6 bytes data in duplex mode where prepend count is
zero. So the extra 3 tx bytes while receiving the first 3 bytes may
disturb the device as they are not expected. It may or may not work. I
would rather just fallback to dummy cs workaround instead of causing
potentially issue.
>> + break;
>> + }
>> + }
>> +
>> + if (list_is_last(&t->transfer_list, &msg->transfers)) {
>> + memcpy(t_prepend, t, sizeof(struct spi_transfer));
>> +
>> + if (tx_only && t->tx_nbits == SPI_NBITS_SINGLE) {
>> + /*
>> + * if the last one is also a single bit tx only transfer, merge
>> + * all of them into one single tx transfer
>> + */
>> + t_prepend->len = bs->prepend_cnt;
>> + t_prepend->tx_buf = bs->prepend_buf;
>> + bs->prepend_cnt = 0;
>> + } else {
>> + /*
>> + * if the last one is not a tx only transfer or dual tx xfer, all
>> + * the previous transfers are sent through prepend bytes and
>> + * make sure it does not exceed the max prepend len
>> + */
>> + if (bs->prepend_cnt > HSSPI_MAX_PREPEND_LEN) {
>> + dev_warn(&bs->pdev->dev,
>> + "exceed max prepend len, abort prepending transfers!\n");
>> + break;
>
> Likewise, you can merge any amount or rx/tx/rxtx single bit transfers
> together as a duplex transfer with prepend len set to 0 (so
> technically not a prepend anymore ;-)
Same here. Let's just fallback to dummy cs workaround mode.
>
>> + }
>> + }
>> + prepend = true;
>> + }
>> + }
>> +
>> + return prepend;
>
> and then if you already returned false if you cannot do prepend, you
> just need to return true here and don't need the prepend variable.
>
>> +}
>> +
>>
>> static int bcm63xx_hsspi_setup(struct spi_device *spi)
>> @@ -344,9 +578,23 @@ static int bcm63xx_hsspi_transfer_one(struct spi_master *master,
>> int status = -EINVAL;
>> int dummy_cs;
>> bool restore_polarity = true;
>> + struct spi_transfer t_prepend;
>>
>> mutex_lock(&bs->msg_mutex);
>> - /* This controller does not support keeping CS active during idle.
>> + if (bcm63xx_check_msg_prependable(master, msg, &t_prepend)) {
>> + status = bcm63xx_hsspi_do_prepend_txrx(spi, &t_prepend);
>> + msg->actual_length += (t_prepend.len + bs->prepend_cnt);
>
> why +=? shouldn't this be the only place in this case where this is set?
>
probably copy/paste error from the dummy cs loop. Will fix.
>> + goto msg_done;
>> + }
>> +
>> + if (bs->xfer_mode == HSSPI_XFER_MODE_PREPEND) {
>> + dev_warn(&bs->pdev->dev,
>> + "User set prepend mode but msg not prependable! Fail the xfer!\n");
>
> If we are failing, this should be a dev_err, not a dev_warn
>
Will fix.
>> + goto msg_done;
>> + }
>
> I think from a readability standpoint it would be better to move the
> cs_workaround parts into their own function, and have this as
>
> bool prependable = false;
>
> if (bs->xfer_mode != HSSPI_XFER_MODE_DUMMYCS)
> prependable = bcm63xx_prepare_prepend_transfer(...);
>
> if (prependable) {
> status = bcm63xx_hsspi_do_prepend_txrx(...);
> msg->actual_legth += ...;
> } else {
> if (bs->xfer_mode == HSSPI_XFER_MODE_PREPEND) {
> /* we may not use dummy cs */
> dev_err(...);
> status = -EINVAL;
> } else {
> status = bcm63xx_hsspi_do_dummy_cs_txrx(...);
> }
> }
>
> with (bcm63xx_hsspi_do_dummy_cs_txrx being the proposed function name).
>
Sound good to me.
>> +
>> + /*
>> + * This controller does not support keeping CS active during idle.
>> * To work around this, we use the following ugly hack:
>> *
>> * a. Invert the target chip select's polarity so it will be active.
>> @@ -364,6 +612,17 @@ static int bcm63xx_hsspi_transfer_one(struct spi_master *master,
>> bcm63xx_hsspi_set_cs(bs, dummy_cs, true);
>>
>> list_for_each_entry(t, &msg->transfers, transfer_list) {
>> + /*
>> + * We are here because one of reasons below:
>> + * a. Message is not prependable and in default auto xfer mode. This mean
>> + * we fallback to dummy cs mode at maximum 25MHz safe clock rate.
>> + * b. User set to use the dummy cs mode.
>> + */
>> + if (bs->xfer_mode == HSSPI_XFER_MODE_AUTO) {
>> + if (t->speed_hz > HSSPI_MAX_SYNC_CLOCK)
>> + t->speed_hz = HSSPI_MAX_SYNC_CLOCK;
>
> OTOH, this may be a point where a dev_warn (once?) might be a good
> idea, since the device may depend on a certain speed to avoid buffer
> overruns (e.g. audio streams - not sure if that exists), so a warning
> that the transfer speed was reduced will help identifying the source.
>
>
That make sense. Should add a warning here.
>
>> + }
>> +
>> status = bcm63xx_hsspi_do_txrx(spi, t);
>> if (status)
>> break;
Download attachment "smime.p7s" of type "application/pkcs7-signature" (4212 bytes)
Powered by blists - more mailing lists