linux-kernel - Re: [PATCH v3] mfd:rtsx: do retry when dma transfer error

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: <20170410150015.o7d2iueesouzfsuh@dell>
Date:   Mon, 10 Apr 2017 16:00:15 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     冯伟linux <steven_feng@...lsil.com.cn>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] mfd:rtsx: do retry when dma transfer error

On Mon, 10 Apr 2017, 冯伟linux wrote:

> > --- a/drivers/mfd/rtsx_pcr.c
> > +++ b/drivers/mfd/rtsx_pcr.c
> > @@ -30,6 +30,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/mfd/core.h>
> >  #include <linux/mfd/rtsx_pci.h>
> > +#include <linux/mmc/card.h>
> > Why is this required?
> >
> The UHS_SER104_MAX_DTR which is in "card_clock = UHS_SER104_MAX_DTR
>  - (pcr->dma_error_count *20000000)" is defined in linux/mmc/card.h, so
> it is required.

Okay.

> >  	spin_lock_irqsave(&pcr->lock, flags);
> > -	if (pcr->trans_result == TRANS_RESULT_FAIL)
> > -		err = -EINVAL;
> > +	if (pcr->trans_result == TRANS_RESULT_FAIL) {
> > +		err = -EILSEQ;
> > "Illegal byte sequence", really?
> >
> This errno need to be -EILSEQ.

You need to explain why.

> >>> +	if (card_clock == UHS_SDR104_MAX_DTR &&
> >>> +		pcr->dma_error_count &&
> >>> +		PCI_PID(pcr) == RTS5227_DEVICE_ID)
> >>> +		card_clock = (UHS_SDR104_MAX_DTR -
> >>> +			pcr->dma_error_count * 20000000);
> > ... but won't this only reduce the clock frequency just once?
> >
> > There is no point bracketing the whole statement.
> >
> > But you do need to bracket one (the second) section of it.
> >
> The times of DMA transfer error occurrs recorded in dma_error_count,
> When DMA transfer error occurrs, the card_clock is reduced by 20MHz.

I think you'll find this logic will only reduce the clock frequency by
20MHz once and only once.

After the first:

  card_clock = (UHS_SDR104_MAX_DTR - pcr->dma_error_count * 20000000)

... happens, the first comparison:

  card_clock == UHS_SDR104_MAX_DTR

... will fail on subsequent attempts and will not allow it to be
reduced any further.  Did you test it?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog