netdev - Re: [PATCH v2 02/14] net: wwan: t7xx: Add control DMA interface

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, 18 Nov 2021 22:36:32 -0800
From:   "Martinez, Ricardo" <ricardo.martinez@...ux.intel.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     netdev@...r.kernel.org, linux-wireless@...r.kernel.org,
        kuba@...nel.org, davem@...emloft.net, johannes@...solutions.net,
        ryazanov.s.a@...il.com, loic.poulain@...aro.org,
        m.chetan.kumar@...el.com, chandrashekar.devegowda@...el.com,
        linuxwwan@...el.com, chiranjeevi.rapolu@...ux.intel.com,
        haijun.liu@...iatek.com, amir.hanania@...el.com,
        dinesh.sharma@...el.com, eliot.lee@...el.com,
        mika.westerberg@...ux.intel.com, moises.veleta@...el.com,
        pierre-louis.bossart@...el.com, muralidharan.sethuraman@...el.com,
        Soumya.Prakash.Mishra@...el.com, sreehari.kancharla@...el.com,
        suresh.nagaraj@...el.com
Subject: Re: [PATCH v2 02/14] net: wwan: t7xx: Add control DMA interface


On 11/1/2021 7:03 AM, Andy Shevchenko wrote:
> On Sun, Oct 31, 2021 at 08:56:23PM -0700, Ricardo Martinez wrote:
>> From: Haijun Lio <haijun.liu@...iatek.com>
>>
>> Cross Layer DMA (CLDMA) Hardware interface (HIF) enables the control
>> path of Host-Modem data transfers. CLDMA HIF layer provides a common
>> interface to the Port Layer.
>>
>> CLDMA manages 8 independent RX/TX physical channels with data flow
>> control in HW queues. CLDMA uses ring buffers of General Packet
>> Descriptors (GPD) for TX/RX. GPDs can represent multiple or single
>> data buffers (DB).
>>
>> CLDMA HIF initializes GPD rings, registers ISR handlers for CLDMA
>> interrupts, and initializes CLDMA HW registers.
>>
>> CLDMA TX flow:
>> 1. Port Layer write
>> 2. Get DB address
>> 3. Configure GPD
>> 4. Triggering processing via HW register write
>>
>> CLDMA RX flow:
>> 1. CLDMA HW sends a RX "done" to host
>> 2. Driver starts thread to safely read GPD
>> 3. DB is sent to Port layer
>> 4. Create a new buffer for GPD ring
...
>
>> +void cldma_hw_reset(void __iomem *ao_base)
>> +{
>> +	iowrite32(ioread32(ao_base + REG_INFRA_RST4_SET) | RST4_CLDMA1_SW_RST_SET,
>> +		  ao_base + REG_INFRA_RST4_SET);
>> +	iowrite32(ioread32(ao_base + REG_INFRA_RST2_SET) | RST2_CLDMA1_AO_SW_RST_SET,
>> +		  ao_base + REG_INFRA_RST2_SET);
>> +	udelay(1);
>> +	iowrite32(ioread32(ao_base + REG_INFRA_RST4_CLR) | RST4_CLDMA1_SW_RST_CLR,
>> +		  ao_base + REG_INFRA_RST4_CLR);
>> +	iowrite32(ioread32(ao_base + REG_INFRA_RST2_CLR) | RST2_CLDMA1_AO_SW_RST_CLR,
>> +		  ao_base + REG_INFRA_RST2_CLR);
> Setting and clearing are in the same order, is it okay?
> Can we do it rather symmetrical?
In this case, order does not matter.

This will be symmetrical in the next iteration.

>> +}
> ...
>
>> +	mb(); /* prevents outstanding GPD updates */
> Is there any counterpart of this barrier?

This is not needed, removing it.

...

>
>> +		ret = cldma_gpd_rx_from_queue(queue, budget, &over_budget);
>> +		if (ret == -ENODATA)
>> +			return 0;
>> +
>> +		if (ret)
>> +			return ret;
> Drop redundant blank line

The style followed is to keep a blank line after 'if' blocks.

Is that acceptable as long as it is consistent across the driver?
>
>> +			/* greedy mode */
>> +			l2_rx_int = cldma_hw_int_status(hw_info, BIT(queue->index), true);
...
>
>> +exit:
> Seems useless.

This tag is used when the PM patch is introduced later in the same series.

> +	return ret;
...