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]

Message-ID: <ZMlLjg9UBi3QO/qV@matsya>
Date:   Tue, 1 Aug 2023 23:44:38 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     Martin Povišer <povik+lin@...ebit.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 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?

> +	 */
> +	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

> +};
> +
> +#define SIO_NTAGS		16
> +
> +typedef void (*sio_ack_callback)(struct sio_chan *, void *, bool);

any reason not to use dmaengine callbacks?

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

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

-- 
~Vinod