linux-kernel - Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

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

Date:   Tue, 2 Apr 2019 23:10:24 +0300
From:   Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:     masonccyang@...c.com.tw
Cc:     bbrezillon@...nel.org, broonie@...nel.org,
        devicetree@...r.kernel.org,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Simon Horman <horms@...ge.net.au>, juliensu@...c.com.tw,
        lee.jones@...aro.org, linux-kernel@...r.kernel.org,
        linux-renesas-soc@...r.kernel.org, linux-spi@...r.kernel.org,
        marek.vasut@...il.com, mark.rutland@....com, robh+dt@...nel.org,
        zhengxunli@...c.com.tw
Subject: Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller
 driver

Hello!

On 04/02/2019 08:48 AM, masonccyang@...c.com.tw wrote:

>> > +static void rpc_spi_mem_set_prep_op_cfg(struct spi_device *spi,
>> > +               const struct spi_mem_op *op,
>> > +               u64 *offs, size_t *len)
>> > +{
>> > +   struct rpc_spi *rpc = spi_controller_get_devdata(spi->controller);
>> > +
>> > +   rpc->cmd = RPC_SMCMR_CMD(op->cmd.opcode);
>> > +   rpc->smenr = RPC_SMENR_CDE |
>> > +           RPC_SMENR_CDB(ilog2(op->cmd.buswidth));
>> > +   rpc->totalxferlen = 1;
>> > +   rpc->xfer_dir = SPI_MEM_NO_DATA;
>> > +   rpc->xferlen = 0;
>> > +   rpc->addr = 0;
>> > +
>> > +   if (op->addr.nbytes) {
>> > +      rpc->smenr |= RPC_SMENR_ADB(ilog2(op->addr.buswidth));
>> > +      if (op->addr.nbytes == 4)
>> > +         rpc->smenr |= RPC_SMENR_ADE(0xf);
>> > +      else
>> > +         rpc->smenr |= RPC_SMENR_ADE(0x7);
>> > +
>> > +      if (offs && len)
>> > +         rpc->addr = *offs;
>> > +      else
>> > +         rpc->addr = op->addr.val;
>> > +      rpc->totalxferlen += op->addr.nbytes;
>> > +   }
>> > +
>> > +   if (op->dummy.nbytes) {
>> > +      rpc->smenr |= RPC_SMENR_DME;
>> > +      rpc->dummy = RPC_SMDMCR_DMCYC(op->dummy.nbytes);
>>
>>    So you haven't fixed this bug? I repeat, the driver doesn't work right
>> w/o this fixed!
> 
> Do you mean
> 
> rpc->dummy = RPC_SMDMCR_DMCYC(op->dummy.nbytes) * 8;   ?

   Yes. Or some other code that amounts to it.

> What is your flash part number?

   Spansion/Cypress S25FS512S. The datasheet says there must be 8 dummy cycles
for the RSFDP command...

> The problem you found is in 1 bit or 4 bits width ?

   1-bit, I think.

>>
>> [...]
>> > +static ssize_t rpc_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
>> > +                   u64 offs, size_t len, void *buf)
>> > +{
>> > +   struct rpc_spi *rpc =
>> > +         spi_controller_get_devdata(desc->mem->spi->controller);
>> > +   int ret;
>> > +
>> > +   if (offs + desc->info.offset + len > U32_MAX)
>> > +      return -EINVAL;
>> > +
>> > +   if (len > 0x4000000)
>> > +      len = 0x4000000;
>>
>>    Ugh...
> 
> by mtd->size ?

  That'd be better, yes.

>> > +
>> > +   ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
>> > +   if (ret)
>> > +      return ret;
>> > +
>> > +   rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
>> > +                &desc->info.op_tmpl, &offs, &len);
>> > +
>> > +   regmap_update_bits(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD, 0);
>> > +   regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RBURST(32) |
>> > +           RPC_DRCR_RBE);
>> > +
>> > +   regmap_write(rpc->regmap, RPC_DRCMR, rpc->cmd);
>> > +   regmap_write(rpc->regmap, RPC_DREAR, RPC_DREAR_EAC(1));
>>
>>    So you're not even trying to support flashes larger than the read dirmap?
>> Now I don't think it's acceptable (and I have rewritten this code internally).
> 
> what about the size comes form mtd->size ?

   I'm not talking about size here; we should use the full address. I'm attaching
my patch...

>> [...]
>> > +static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
>> > +               u64 offs, size_t len, const void *buf)
>> > +{
>> > +   struct rpc_spi *rpc =
>> > +         spi_controller_get_devdata(desc->mem->spi->controller);
>> > +   int ret;
>> > +
>> > +   if (offs + desc->info.offset + len > U32_MAX)
>> > +      return -EINVAL;
>> > +
>> > +   if (len > RPC_WBUF_SIZE)
>> > +      len = RPC_WBUF_SIZE;
>>
>>    Ugh! Again, I no longer think this is acceptable... Maybe we just need
>> to drop the support of the write buffer...
>>
> 
> why ?
> Using write buffer got the much better write performance.

   OK, let's keep it then.

[...]

MBR, Sergei

View attachment "spi-renesas-rpc-allow-reading-from-large-flashes.patch" of type "text/x-patch" (2600 bytes)