linux-kernel - Re: [PATCH v4 19/23] drivers/fsi: Add GPIO based FSI master

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: <1490907014.3177.207.camel@kernel.crashing.org>
Date:   Fri, 31 Mar 2017 07:50:14 +1100
From:   Benjamin Herrenschmidt <benh@...nel.crashing.org>
To:     Christopher Bostic <cbostic@...ux.vnet.ibm.com>,
        Joel Stanley <joel@....id.au>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Russell King <linux@...linux.org.uk>, rostedt@...dmis.org,
        mingo@...hat.com, Greg KH <gregkh@...uxfoundation.org>,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andrew Jeffery <andrew@...id.au>,
        Alistair Popple <alistair@...ple.id.au>,
        "Edward A . James" <eajames@...ibm.com>,
        Jeremy Kerr <jk@...abs.org>
Subject: Re: [PATCH v4 19/23] drivers/fsi: Add GPIO based FSI master

On Thu, 2017-03-30 at 13:15 -0500, Christopher Bostic wrote:
> > > +static void serial_in(struct fsi_master_gpio *master, struct fsi_gpio_msg *msg,
> > > +                       uint8_t num_bits)
> > > +{
> > > +       uint8_t bit, in_bit;
> > > +
> > > +       set_sda_input(master);
> > > +
> > > +       for (bit = 0; bit < num_bits; bit++) {
> > > +               clock_toggle(master, 1);
> > > +               in_bit = sda_in(master);
> > > +               msg->msg <<= 1;
> > > +               msg->msg |= ~in_bit & 0x1;      /* Data is negative active */
> > > +       }
> > > +       msg->bits += num_bi	ts;
> > > +}
> > > +
> > > +static void serial_out(struct fsi_master_gpio *master,
> > > +                       const struct fsi_gpio_msg *cmd)
> > > +{
> > > +       uint8_t bit;
> > > +       uint64_t msg = ~cmd->msg;       /* Data is negative active */
> > > +       uint64_t sda_mask = 0x1ULL << (cmd->bits - 1);
> > > +       uint64_t last_bit = ~0;
> > > +       int next_bit;
> > > +
> > > +       if (!cmd->bits) {
> > > +               dev_warn(master->dev, "trying to output 0 bits\n");
> > > +               return;
> > > +       }
> > > +       set_sda_output(master, 0);
> > > +
> > > +       /* Send the start bit */
> > > +       sda_out(master, 0);
> > > +       clock_toggle(master, 1);
> > > +
> > > +       /* Send the message */
> > > +       for (bit = 0; bit < cmd->bits; bit++) {
> > > +               next_bit = (msg & sda_mask) >> (cmd->bits - 1);
> > > +               if (last_bit ^ next_bit) {
> > > +                       sda_out(master, next_bit);
> > > +                       last_bit = next_bit;
> > > +               }
> > > +               clock_toggle(master, 1);
> > > +               msg <<= 1;
> > > +       }
> > > +}

As I mentioned privately, I don't think this is right, unless your
clock signal is inverted or my protocol spec is wrong...

Your clock toggle is written so you call it right after the rising
edge. It does delay, 0, delay, 1.

But according to the FSI timing diagram I have, you need to establish
the data around the falling edge, it gets sampled by the slave on the
rising edge. So as it is, your code risks violating the slave hold
time.

On input, you need to sample on the falling edge, right before it. You
are sampling after the rising edge, so you aren't leaving enough time
for the slave to establish the data.

You could probably just flip clock_toggle() around. Make it: 0, delay,
1, delay.

That way you can do for sends: sda_out + toggle, and for receive
toggle + sda_in. That will make you establish your output data and
sample right before the falling edge, which should be ok provided the
diagram I have is right.

Cheers,
Ben.