From: "Martin Povišer" <povik+lin@cutebit.org>
To: Vinod Koul <vkoul@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>,
asahi@lists.linux.dev, dmaengine@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] dmaengine: apple-sio: Add Apple SIO driver
Date: Thu, 24 Aug 2023 17:25:41 +0200 [thread overview]
Message-ID: <831F5957-18B4-4EB9-A755-0E5FE58A83E5@cutebit.org> (raw)
In-Reply-To: <ZMuQvhIg0AHH2e7V@matsya>
> On 3. 8. 2023, at 13:34, Vinod Koul <vkoul@kernel.org> wrote:
>
> On 03-08-23, 10:32, Martin Povišer wrote:
>
>>>>> +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?
>
> yeah xarray am not too sure. I would still go with ida, we will see when
> it is relly removed.
Sorry for letting this sleep for a while.
I don’t like the idea of submitting a new driver to use a deprecated
interface. For all I know someone can come along later and mark the driver
as broken in the process of finally removing IDA, with good excuse to do so.
> If you need a bitfield why not use bitmap apis.
> I dont like drivers implementing the basic logic which kernel provides
I think one improvement to take up is to use the DECLARE_BITMAP macro for
the `allocated` bitmap. Other than that this already uses the bitmap.h/
bitops.h functions to the degree it can if the goal is to
(1) allocate and free the tags reliably under SMP with atomic ops
(2) in best-effort manner (but without locking of the counter) make
the tag numbers consecutive
The latter behaviour is there to make traces easier to read.
Martin
> --
> ~Vinod
next prev parent reply other threads:[~2023-08-24 15:31 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-12 13:38 [PATCH 0/2] Apple SIO driver Martin Povišer
2023-07-12 13:38 ` [PATCH 1/2] dt-bindings: dma: apple,sio: Add schema Martin Povišer
2023-07-12 13:38 ` [PATCH 2/2] dmaengine: apple-sio: Add Apple SIO driver Martin Povišer
2023-08-01 18:14 ` Vinod Koul
2023-08-01 21:55 ` Martin Povišer
2023-08-03 8:32 ` Martin Povišer
2023-08-03 11:34 ` Vinod Koul
2023-08-24 15:25 ` Martin Povišer [this message]
2023-08-03 11:25 ` Vinod Koul
2023-08-24 15:34 ` Martin Povišer
2023-07-12 19:42 ` [PATCH 0/2] " Krzysztof Kozlowski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=831F5957-18B4-4EB9-A755-0E5FE58A83E5@cutebit.org \
--to=povik+lin@cutebit.org \
--cc=asahi@lists.linux.dev \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dmaengine@vger.kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=vkoul@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).