linux-kernel - Re: [PATCH 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]

Date:   Thu, 24 Aug 2023 17:25:41 +0200
From:   Martin Povišer <povik+lin@...ebit.org>
To:     Vinod Koul <vkoul@...nel.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 2/2] dmaengine: apple-sio: Add Apple SIO driver


> On 3. 8. 2023, at 13:34, Vinod Koul <vkoul@...nel.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