linux-kernel - Re: [PATCH] mmc: add TS7800 FPGA based MMC controller 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-next>] [day] [month] [year] [list]

Date:   Thu, 20 Oct 2022 10:42:50 -0400
From:   firas ashkar <firas.ashkar@...oirfairelinux.com>
To:     Arnd Bergmann <arnd@...db.de>, alex@...riz.org.uk,
        Ulf Hansson <ulf.hansson@...aro.org>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-mmc@...r.kernel.org,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mmc: add TS7800 FPGA based MMC controller driver

thank you for the comments, pls check below for replies and acks 
-- 
Firas Ashkar
Developpeur Système Embarqué

savoirfairelinux.com | Montréal, Québec

Tél.: +1 514 276 5468 ext. 118


On Tue, 2022-10-18 at 21:06 +0200, Arnd Bergmann wrote:
> On Tue, Oct 18, 2022, at 15:38, Firas Ashkar wrote:
> > add standard mmc/host controller driver for TS-7800v1, instead of
> > the
> > original block based 'tssdcore' driver provided by EmbeddedTS
> > linux-2.6.x
> > code base.
> 
> I'm looking at this driver since Marc pointed me to your
> work on the platform and I noticed this post as well.
> Not doing a full review, but I'm still pointing out a few
> issue that caught my eye.
> 
> > +
> > +#define DRIVER_NAME "ts7800v1_sdmmc"
> 
> Maybe drop this macro and just use the string directly.
This define macro used in multiple locations
> 
> > +
> > +       if (IS_ERR_OR_NULL(pslot->rw_dma_buf)) {
> > +               dev_warn(mmc_dev(ts_sdmmc_host->mmc_host),
> > +                        "%s|%d - Error, No allocated DMA read
> > buffer %ld\n",
> > +                        __func__, __LINE__, PTR_ERR(pslot-
> > >rw_dma_buf));
> > +               *data_error = ret = -ENOMEM;
> > +               goto done;
> > +       }
> > +
> > +       dat0_sent_crc16_buf = kzalloc(sizeof(u16), GFP_KERNEL);
> > +       if (IS_ERR_OR_NULL(dat0_sent_crc16_buf)) {
> 
> You should never need IS_ERR_OR_NULL, as all interfaces
> in the kernel are supposed to either return an error code
> or return NULL on error. Please fix the error handling
> for this throughout the driver.
ack
> 
> > + spin_lock_bh(&ts_sdmmc_host->bh_lock);
> 
> I'm a bit confused by your locking. Why do you use
> spin_lock_bh() instead of a normal spin_lock() or
> a spin_lock_irq()? I don't see any use of softirqs
> (typically tasklets and timers) in this driver, so
> disabling softirqs in the critical section should
> not change anything.
ack
> 
>      Arnd