linux-kernel - Re: [PATCH v2 2/2] dmaengine: apple-sio: Add Apple SIO 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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZR1smXBXyx7xDEmg@matsya>
Date:   Wed, 4 Oct 2023 19:16:01 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     Martin Povišer <povik+lin@...ebit.org>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>, asahi@...ts.linux.dev,
        dmaengine@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] dmaengine: apple-sio: Add Apple SIO driver

On 04-10-23, 15:32, Martin Povišer wrote:

> >> + * There are two kinds of 'transaction descriptors' in play here.
> >> + *
> >> + * There's the struct sio_tx, and the struct dma_async_tx_descriptor embedded
> >> + * inside, which jointly represent a transaction to the dmaengine subsystem.
> >> + * At this time we only support those transactions to be cyclic.
> >> + *
> >> + * Then there are the coprocessor descriptors, which is what the coprocessor
> >> + * knows and understands. These don't seem to have a cyclic regime, so we can't
> >> + * map the dmaengine transaction on an exact coprocessor counterpart. Instead
> >> + * we continually queue up many coprocessor descriptors to implement a cyclic
> >> + * transaction.
> >> + *
> >> + * The number below is the maximum of how far ahead (how many) coprocessor
> >> + * descriptors we should be queuing up, per channel, for a cyclic transaction.
> >> + * Basically it's a made-up number.
> >> + */
> >> +#define SIO_MAX_NINFLIGHT 4
> > 
> > you meant SIO_MAX_INFLIGHT if not what is NINFLIGHT?
> 
> I mean the number is arbitrary, it doesn’t reflect any coprocessor limit since
> I haven’t run the tests to figure one out. It's supposed to be a small reasonable
> number.

Sorry that was not my question. Should this macro be SIO_MAX_NINFLIGHT
or SIO_MAX_INFLIGHT..?

> >> +static int sio_device_config(struct dma_chan *chan,
> >> +      struct dma_slave_config *config)
> >> +{
> >> + struct sio_chan *siochan = to_sio_chan(chan);
> >> + struct sio_data *sio = siochan->host;
> >> + bool is_tx = sio_chan_direction(siochan->no) == DMA_MEM_TO_DEV;
> >> + struct sio_shmem_chan_config *cfg = sio->shmem;
> >> + int ret;
> >> +
> >> + switch (is_tx ? config->dst_addr_width : config->src_addr_width) {
> >> + case DMA_SLAVE_BUSWIDTH_1_BYTE:
> >> + cfg->datashape = 0;
> >> + break;
> >> + case DMA_SLAVE_BUSWIDTH_2_BYTES:
> >> + cfg->datashape = 1;
> >> + break;
> >> + case DMA_SLAVE_BUSWIDTH_4_BYTES:
> >> + cfg->datashape = 2;
> >> + break;
> >> + default:
> >> + return -EINVAL;
> >> + }
> >> +
> >> + cfg->fifo = 0x800;
> >> + cfg->limit = 0x800;
> >> + cfg->threshold = 0x800;
> >> + dma_wmb();
> > 
> > ??
> 
> Again, shared memory
> 
> >> +
> >> + ret = sio_call(sio, FIELD_PREP(SIOMSG_TYPE, MSG_CONFIGURE) |
> >> +     FIELD_PREP(SIOMSG_EP, siochan->no));
> > 
> > this does not sound okay, can you explain why this call is here
> 
> We are sending the configuration to the coprocessor, it will NACK
> it if invalid, seems very fitting here.

I dont this so, purpose of the device_config() is to send peripheral
config to driver for use on the next descriptor which is submitted. So
sending to co-processor now (when we might even have a txn going on)
does not seem right

What would be the behaviour if already a txn is progressing on the
co-processor

-- 
~Vinod