| 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
| ||
|
Message-ID: <bab1eec0-8b1d-6005-e9eb-05e93da844aa@nvidia.com>
Date: Tue, 28 Jan 2020 14:05:17 +0000
From: Jon Hunter <jonathanh@...dia.com>
To: Dmitry Osipenko <digetx@...il.com>,
Laxman Dewangan <ldewangan@...dia.com>,
Vinod Koul <vkoul@...nel.org>,
Dan Williams <dan.j.williams@...el.com>,
Thierry Reding <thierry.reding@...il.com>,
Michał Mirosław <mirq-linux@...e.qmqm.pl>
CC: <dmaengine@...r.kernel.org>, <linux-tegra@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 08/14] dmaengine: tegra-apb: Fix coding style problems
On 16/01/2020 17:37, Dmitry Osipenko wrote:
> 15.01.2020 12:49, Jon Hunter пишет:
>>
>>
>> On 12/01/2020 17:30, Dmitry Osipenko wrote:
>>> This patch fixes few dozens of coding style problems reported by
>>> checkpatch and prettifies code where makes sense.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@...il.com>
>>> ---
>>> drivers/dma/tegra20-apb-dma.c | 276 ++++++++++++++++++----------------
>>> 1 file changed, 144 insertions(+), 132 deletions(-)
>>>
>>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>>> index dff21e80ffa4..7158bd3145c4 100644
>>> --- a/drivers/dma/tegra20-apb-dma.c
>>> +++ b/drivers/dma/tegra20-apb-dma.c
>>
>> ...
>>
>>> @@ -1003,20 +1014,23 @@ static void tegra_dma_prep_wcount(struct tegra_dma_channel *tdc,
>>> ch_regs->csr |= len_field;
>>> }
>>>
>>> -static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>>> - struct dma_chan *dc, struct scatterlist *sgl, unsigned int sg_len,
>>> - enum dma_transfer_direction direction, unsigned long flags,
>>> - void *context)
>>> +static struct dma_async_tx_descriptor *
>>> +tegra_dma_prep_slave_sg(struct dma_chan *dc,
>>> + struct scatterlist *sgl,
>>> + unsigned int sg_len,
>>> + enum dma_transfer_direction direction,
>>> + unsigned long flags,
>>> + void *context)
>>> {
>>> struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>>> + struct tegra_dma_sg_req *sg_req = NULL;
>>> + u32 csr, ahb_seq, apb_ptr, apb_seq;
>>> + enum dma_slave_buswidth slave_bw;
>>> struct tegra_dma_desc *dma_desc;
>>> - unsigned int i;
>>> - struct scatterlist *sg;
>>> - unsigned long csr, ahb_seq, apb_ptr, apb_seq;
>>> struct list_head req_list;
>>> - struct tegra_dma_sg_req *sg_req = NULL;
>>> - u32 burst_size;
>>> - enum dma_slave_buswidth slave_bw;
>>> + struct scatterlist *sg;
>>> + unsigned int burst_size;
>>> + unsigned int i;
>>
>> This is not really consistent with the rest of the changes by having 'i'
>> and 'burst_size' on separate lines.
>
> The goal wasn't to squash everything into a single line, but to make
> code more readable. In this particular case the separated lines look
> better to me.
>
>>>
>>> if (!tdc->config_init) {
>>> dev_err(tdc2dev(tdc), "DMA channel is not configured\n");
>>> @@ -1028,7 +1042,7 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>>> }
>>>
>>> if (get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
>>> - &burst_size, &slave_bw) < 0)
>>> + &burst_size, &slave_bw) < 0)
>>> return NULL;
>>>
>>> INIT_LIST_HEAD(&req_list);
>>> @@ -1074,7 +1088,7 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>>> len = sg_dma_len(sg);
>>>
>>> if ((len & 3) || (mem & 3) ||
>>> - (len > tdc->tdma->chip_data->max_dma_count)) {
>>> + len > tdc->tdma->chip_data->max_dma_count) {
>>> dev_err(tdc2dev(tdc),
>>> "DMA length/memory address is not supported\n");
>>> tegra_dma_desc_put(tdc, dma_desc);
>>> @@ -1126,20 +1140,21 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>>> return &dma_desc->txd;
>>> }
>>>
>>> -static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
>>> - struct dma_chan *dc, dma_addr_t buf_addr, size_t buf_len,
>>> - size_t period_len, enum dma_transfer_direction direction,
>>> - unsigned long flags)
>>> +static struct dma_async_tx_descriptor *
>>> +tegra_dma_prep_dma_cyclic(struct dma_chan *dc, dma_addr_t buf_addr,
>>> + size_t buf_len,
>>> + size_t period_len,
>>> + enum dma_transfer_direction direction,
>>> + unsigned long flags)
>>> {
>>> struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>>> - struct tegra_dma_desc *dma_desc = NULL;
>>> struct tegra_dma_sg_req *sg_req = NULL;
>>> - unsigned long csr, ahb_seq, apb_ptr, apb_seq;
>>> - int len;
>>> - size_t remain_len;
>>> - dma_addr_t mem = buf_addr;
>>> - u32 burst_size;
>>> + u32 csr, ahb_seq, apb_ptr, apb_seq;
>>> enum dma_slave_buswidth slave_bw;
>>> + struct tegra_dma_desc *dma_desc;
>>> + dma_addr_t mem = buf_addr;
>>> + unsigned int burst_size;
>>> + size_t len, remain_len;
>>>
>>> if (!buf_len || !period_len) {
>>> dev_err(tdc2dev(tdc), "Invalid buffer/period len\n");
>>> @@ -1173,13 +1188,13 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
>>>
>>> len = period_len;
>>> if ((len & 3) || (buf_addr & 3) ||
>>> - (len > tdc->tdma->chip_data->max_dma_count)) {
>>> + len > tdc->tdma->chip_data->max_dma_count) {
>>> dev_err(tdc2dev(tdc), "Req len/mem address is not correct\n");
>>> return NULL;
>>> }
>>>
>>> if (get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
>>> - &burst_size, &slave_bw) < 0)
>>> + &burst_size, &slave_bw) < 0)
>>> return NULL;
>>>
>>> ahb_seq = TEGRA_APBDMA_AHBSEQ_INTR_ENB;
>>> @@ -1269,7 +1284,6 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
>>> int ret;
>>>
>>> dma_cookie_init(&tdc->dma_chan);
>>> - tdc->config_init = false;
>>
>> Why is this removed? Does not seem to belong in this patch.
>
> Because initially, on driver's probe, the tdc->config_init is false for
> all channels and then tegra_dma_free_chan_resources() also sets it to
> false. Thus there is no need to re-initilize the already initialized
> variable. It's not a very good coding style if variables are
> unnecessarily initialized, you probably noticed that there are few other
> cases of removing the unneeded initializations of local variables in
> this patch.
OK, but I don't really consider this coding-style and would prefer a
separate patch for this.
Jon
--
nvpublic
Powered by blists - more mailing lists