Re: [PATCH 2/2] dmaengine: apple-sio: Add Apple SIO driver

[Martin Povišer]

Hi Vinod!

> On 1. 8. 2023, at 20:14, Vinod Koul <vkoul@xxxxxxxxxx> wrote:
> 
> On 12-07-23, 15:38, Martin Povišer wrote:
> 
>> +struct sio_chan {
>> +	unsigned int no;
>> +	struct sio_data *host;
>> +	struct dma_chan chan;
>> +	struct tasklet_struct tasklet;
>> +	struct work_struct terminate_wq;
>> +
>> +	spinlock_t lock;
>> +	struct sio_tx *current_tx;
>> +	/*
>> +	 * 'tx_cookie' is used for distinguishing between transactions from
>> +	 * within tag ack/nack callbacks. Without it, we would have no way
>> +	 * of knowing if the current transaction is the one the callback handler
>> +	 * was installed for.
> 
> not sure what you mean by here.. I dont see why you would need to store
> cookie here, care to explain?

I could have clarified this is not meant to be the dmaengine cookie, just
a driver-level cookie to address a race between

	a dmaengine user calling terminate_all to terminate a running
	cyclic transaction, then issuing a new one

on one hand, and

	the coprocessor acking the issuing of one of the coprocessor
	transactions that correspond to the first dmaengine transaction

on the other hand. With the cookie the driver should not get confused
about which dmaengine transaction the ACK belongs to, since if `current_tx`
changed in the meantime the cookie won’t match.

But now that I look at it... huh, I never increment that `tx_cookie` field!
I don’t know if I have considered using the dmaengine cookie to the same
effect. Maybe we can do that, I see how that would be much desirable.

>> +	 */
>> +	unsigned long tx_cookie;
>> +	int nperiod_acks;
>> +
>> +	/*
>> +	 * We maintain a 'submitted' and 'issued' list mainly for interface
>> +	 * correctness. Typical use of the driver (per channel) will be
>> +	 * prepping, submitting and issuing a single cyclic transaction which
>> +	 * will stay current until terminate_all is called.
>> +	 */
>> +	struct list_head submitted;
>> +	struct list_head issued;
>> +
>> +	struct list_head to_free;
> 
> can you use virt_dma_chan, that should simplify list handling etc

I looked into that when I wrote the sister driver apple-admac.c, I don’t
remember anymore why I decided against it, and I don’t think it came up
during review. Now that this driver is done, I hope we can take it as is.

There’s some benefit from the drivers having a similar structure, I sent
one or two fixes to apple-admac for things I found out because I was
writing this other driver.

>> +};
>> +
>> +#define SIO_NTAGS		16
>> +
>> +typedef void (*sio_ack_callback)(struct sio_chan *, void *, bool);
> 
> any reason not to use dmaengine callbacks?

Not sure what dmaengine callback you mean here. This callback means
the coprocessor acked a tag, not sure how we can fit something dmaengine
onto it.

>> +static int sio_alloc_tag(struct sio_data *sio)
>> +{
>> +	struct sio_tagdata *tags = &sio->tags;
>> +	int tag, i;
>> +
>> +	/*
>> +	 * Because tag number 0 is special, the usable tag range
>> +	 * is 1...(SIO_NTAGS - 1). So, to pick the next usable tag,
>> +	 * we do modulo (SIO_NTAGS - 1) *then* plus one.
>> +	 */
>> +
>> +#define SIO_USABLE_TAGS (SIO_NTAGS - 1)
>> +	tag = (READ_ONCE(tags->last_tag) % SIO_USABLE_TAGS) + 1;
>> +
>> +	for (i = 0; i < SIO_USABLE_TAGS; i++) {
>> +		if (!test_and_set_bit(tag, &tags->allocated))
>> +			break;
>> +
>> +		tag = (tag % SIO_USABLE_TAGS) + 1;
>> +	}
>> +
>> +	WRITE_ONCE(tags->last_tag, tag);
>> +
>> +	if (i < SIO_USABLE_TAGS)
>> +		return tag;
>> +	else
>> +		return -EBUSY;
>> +#undef SIO_USABLE_TAGS
>> +}
> 
> can you use kernel mechanisms like ida to alloc and free the tags...

I can look into that.

>> +static struct dma_async_tx_descriptor *sio_prep_dma_cyclic(
>> +		struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
>> +		size_t period_len, enum dma_transfer_direction direction,
>> +		unsigned long flags)
>> +{
>> +	struct sio_chan *siochan = to_sio_chan(chan);
>> +	struct sio_tx *siotx = NULL;
>> +	int i, nperiods = buf_len / period_len;
>> +
>> +	if (direction != sio_chan_direction(siochan->no))
>> +		return NULL;
>> +
>> +	siotx = kzalloc(struct_size(siotx, siodesc, nperiods), GFP_NOWAIT);
>> +	if (!siotx)
>> +		return NULL;
>> +
>> +	init_completion(&siotx->done);
>> +	dma_async_tx_descriptor_init(&siotx->tx, chan);
>> +	siotx->period_len = period_len;
>> +	siotx->nperiods = nperiods;
>> +
>> +	for (i = 0; i < nperiods; i++) {
>> +		struct sio_coproc_desc *d;
>> +
>> +		siotx->siodesc[i] = d = sio_alloc_desc(siochan->host);
>> +		if (!d) {
>> +			sio_tx_free(&siotx->tx);
>> +			return NULL;
>> +		}
>> +
>> +		d->flag = 1; // not sure what's up with this
>> +		d->iova = buf_addr + period_len * i;
>> +		d->size = period_len;
>> +	}
>> +	dma_wmb();
> 
> why use barrier here? and to what purpose..

Few lines above we are modifying a shared memory buffer that’s mapped into
the coprocessor’s address space (it’s what `d` points to).

> -- 
> ~Vinod
> 

Best regards, Martin