| 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: <20150210035833.GA19195@hsj.sh.intel.com>
Date: Tue, 10 Feb 2015 11:58:33 +0800
From: Huang Shijie <shijie.huang@...el.com>
To: Brian Norris <computersforpeace@...il.com>
Cc: Jim-Ting Kuo <jimtingkuo@...il.com>,
David Woodhouse <dwmw2@...radead.org>,
Marek Vasut <marex@...x.de>,
Geert Uytterhoeven <geert+renesas@...der.be>,
Rafał Miłecki <zajec5@...il.com>,
Ben Hutchings <ben@...adent.org.uk>,
Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>,
linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] m25p80, spi-nor: Update id list of Macronix chips
On Fri, Feb 06, 2015 at 11:09:50AM -0800, Brian Norris wrote:
> (fixed Huang's current email)
>
> Hi,
>
> On Fri, Feb 06, 2015 at 10:33:29PM +0800, Jim-Ting Kuo wrote:
> > On Fri, Feb 6, 2015 at 9:18 AM, Brian Norris
> > <computersforpeace@...il.com> wrote:
> > > On Fri, Feb 06, 2015 at 02:44:04AM +0800, Jim Kuo wrote:
> > >> --- a/drivers/mtd/devices/m25p80.c
> > >> +++ b/drivers/mtd/devices/m25p80.c
> > >> @@ -170,6 +170,74 @@ static int m25p80_erase(struct spi_nor *nor, loff_t offset)
> > >> return 0;
> > >> }
> > >>
> > >> +static int m25p80_write_xfer(struct spi_nor *nor,
> > >> + struct spi_nor_xfer_cfg *cfg, u8 *buf, size_t len)
> > >> +{
> ...
> > >> +}
> > >> +
> > >> +static int m25p80_read_xfer(struct spi_nor *nor,
> > >> + struct spi_nor_xfer_cfg *cfg, u8 *buf, size_t len)
> > >> +{
> ...
> > >> +}
> > >> +
> > >
> > > All of the above is pointless. The write_xfer/read_xfer functions are
> > > not even used. (They should probably just be dropped, since they're
> > > doing nothing as-is.)
> > >
> > >> /*
> > >> * board specific setup should have ensured the SPI clock used here
> > >> * matches what the READ command supports, at least until this driver
> > >> @@ -199,6 +267,8 @@ static int m25p_probe(struct spi_device *spi)
> > >> nor->erase = m25p80_erase;
> > >> nor->write_reg = m25p80_write_reg;
> > >> nor->read_reg = m25p80_read_reg;
> > >> + nor->write_xfer = m25p80_write_xfer;
> > >> + nor->read_xfer = m25p80_read_xfer;
> > >>
> > >> nor->dev = &spi->dev;
> > >> nor->mtd = &flash->mtd;
> > >> @@ -261,10 +331,15 @@ static const struct spi_device_id m25p_ids[] = {
> > >> {"mr25h256"}, {"mr25h10"},
> > >> {"gd25q32"}, {"gd25q64"},
> > >> {"160s33b"}, {"320s33b"}, {"640s33b"},
> > >> - {"mx25l2005a"}, {"mx25l4005a"}, {"mx25l8005"}, {"mx25l1606e"},
> > >> - {"mx25l3205d"}, {"mx25l3255e"}, {"mx25l6405d"}, {"mx25l12805d"},
> > >> - {"mx25l12855e"},{"mx25l25635e"},{"mx25l25655e"},{"mx66l51235l"},
> > >> - {"mx66l1g55g"},
> > >> + {"mx25l512e"}, {"mx25l5121e"}, {"mx25l1006e"}, {"mx25l1021e"},
> > >> + {"mx25l2006e"}, {"mx25l4006e"}, {"mx25u4035"}, {"mx25v4035"},
> > >> + {"mx25l8006e"}, {"mx25u8035"}, {"mx25v8035"}, {"mx25l1606e"},
> > >> + {"mx25l1633e"}, {"mx25l1635e"}, {"mx25u1635e"}, {"mx25l3206e"},
> > >> + {"mx25l3239e"}, {"mx25l3225d"}, {"mx25l3255e"}, {"mx25l6406e"},
> > >> + {"mx25l6439e"}, {"mx25l12839f"}, {"mx25l12855e"},
> > >> + {"mx25u12835f"}, {"mx25l25635e"}, {"mx25l25655e"},
> > >> + {"mx25u25635f"}, {"mx66l51235f"}, {"mx66u51235f"},
> > >> + {"mx66l1g45g"}, {"mx66l1g55g"},
> > >> {"n25q064"}, {"n25q128a11"}, {"n25q128a13"}, {"n25q256a"},
> > >> {"n25q512a"}, {"n25q512ax3"}, {"n25q00"},
> > >> {"pm25lv512"}, {"pm25lv010"}, {"pm25lq032"},
> > >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > >> index 0f8ec3c..a6c7337 100644
> > >> --- a/drivers/mtd/spi-nor/spi-nor.c
> > >> +++ b/drivers/mtd/spi-nor/spi-nor.c
> > >> @@ -545,19 +545,37 @@ static const struct spi_device_id spi_nor_ids[] = {
> > >> { "640s33b", INFO(0x898913, 0, 64 * 1024, 128, 0) },
> > >>
> > >> /* Macronix */
> > >> - { "mx25l2005a", INFO(0xc22012, 0, 64 * 1024, 4, SECT_4K) },
> > >
> > > Nak.
> > >
> > >> - { "mx25l4005a", INFO(0xc22013, 0, 64 * 1024, 8, SECT_4K) },
> > >
> > > Nak.
> > >
> > >> - { "mx25l8005", INFO(0xc22014, 0, 64 * 1024, 16, 0) },
> > >
> > > Nak.
> > >
> > > (you get the picture)
> > >
> [...]
> > >
> > > You need to do a lot better job of documenting your patches (i.e.,
> > > describe why you're doing these things) if you want me to take them.
> > >
> > > What's more, the SFDP support you're trying to add should be done in a
> > > way where you *don't* need to muck with the ID table every time. With
> > > SFDP you can get most/all this information anyway, and devices should
> > > just be able to bind to this driver using a generic name like
> > > "spi-nor,sfdp" or something like that, instead of having to expand this
> > > table.
> >
> > The write_xfer/read_xfer functions actually are used in patch 2.
> > - read_xfer: for macronix_block_lock function.
> > - write_xfer: for sfdp_read, macronix_read_lock_status function.
> > Original read/write fcuntions can only support one type transfer I/O (such as
> > FAST_READ, DUAL_READ or QUAD_READ which was detected at begin),
> > and they also only support fixed cycle of dummy (bind with transfer command).
> > So the commands not related to data transfer, such as sfdp read, lock/unlock
> > are hard to use original read/write. That's why I built these two of functions.
> > I thought they are suitable for this condition.
>
> OK, I suppose that's a little more reasonable. But I want to make sure
> this is actually necessary. I suppose we can't get this functionality
> with the existing read_reg()/write_reg() ops, can we?
>
> I'm not actually sure the exact purpose of the read_xfer()/write_xfer()
> functions as originally defined. They obviously weren't used yet.
> Perhaps Huang can comment here?
The read_xfer()/write_xfer() are designed for the strange spi-nor
controllers which has special optimizations for some operations, such as
combine several operations into one operation.
I am not familiar with SFDP, but I think it will be okay if the SFDP implements the
read_xfer()/write_xfer() if it really can not be implemented by the
read/write/read_reg/write_reg.
please see:
http://lists.infradead.org/pipermail/linux-mtd/2013-November/050492.html
Please split these patch into small patches, and document it well. :)
thanks
Huang Shijie
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists