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

[Martin Povišer]

> On 1. 8. 2023, at 23:55, Martin Povišer <povik+lin@xxxxxxxxxxx> wrote:
> 
> 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.

Indeed nothing is stopping us from matching on the dmaengine cookie to
address the race, so I will be dropping this `tx_cookie` field in v2.

>>> +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.

Documentation says IDA is deprecated in favour of Xarray, both look
like they serve to associate a pointer with an ID. I think neither
structure beats a simple bitfield and a static array for the per-tag
data. Agree?

Martin