> 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
