Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190122185351.GA28627@kadam> <CAPo_4QB+ozVESD85d2qxuitgvgPgLvrjx9SyEMfFTGvkz4MjWg@mail.gmail.com>
 <CAKdAkRT6hKywKW1NnD-hiuFn9YTZF74GZVr36ZNJqyoxQghT9Q@mail.gmail.com>
In-Reply-To: <CAKdAkRT6hKywKW1NnD-hiuFn9YTZF74GZVr36ZNJqyoxQghT9Q@mail.gmail.com>
From:   Ken Sloat <ken.sloat@ohmlinxelectronics.com>
Date:   Sat, 26 Jan 2019 20:20:43 -0500
Message-ID: <CAPo_4QCMX+oYw2ZcBgSiLcEzgTJMJB=E-xdXWe_Xg2a=FmQ+Rg@mail.gmail.com>
Subject: Re: [bug report] Input: add st-keyscan driver
To:     Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc:     Dan Carpenter <dan.carpenter@oracle.com>,
        linux-input <linux-input@vger.kernel.org>,
        Gabriel FERNANDEZ <gabriel.fernandez@st.com>,
        lkml <linux-kernel@vger.kernel.org>,
        Nate Drude <nate.drude@ohmlinxelectronics.com>,
        giuseppe.condorelli@st.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Jan 26, 2019 at 5:15 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Sat, Jan 26, 2019 at 1:25 PM Ken Sloat
> <ken.sloat@ohmlinxelectronics.com> wrote:
> >
> > On Tue, Jan 22, 2019 at 1:53 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > Hello Gabriel FERNANDEZ,
> >
> > Hello Dan,
> >
> > I have added CCs for the maintainers as well as Gabriel Fernandez as
> > currently you just have the linux-input mailing list
> >
> > > The patch 062589b13991: "Input: add st-keyscan driver" from Apr 12,
> > > 2014, leads to the following static checker warning:
> > >
> > >         drivers/input/keyboard/st-keyscan.c:156 keyscan_probe()
> > >         error: potential zalloc NULL dereference: 'keypad_data->input_dev'
> > >
> > > drivers/input/keyboard/st-keyscan.c
> > >     125 static int keyscan_probe(struct platform_device *pdev)
> > >     126 {
> > >     127         struct st_keyscan *keypad_data;
> > >     128         struct input_dev *input_dev;
> > >     129         struct resource *res;
> > >     130         int error;
> > >     131
> > >     132         if (!pdev->dev.of_node) {
> > >     133                 dev_err(&pdev->dev, "no DT data present\n");
> > >     134                 return -EINVAL;
> > >     135         }
> > >     136
> > >     137         keypad_data = devm_kzalloc(&pdev->dev, sizeof(*keypad_data),
> > >     138                                    GFP_KERNEL);
> > >     139         if (!keypad_data)
> > >     140                 return -ENOMEM;
> > >     141
> > >     142         input_dev = devm_input_allocate_device(&pdev->dev);
> > >     143         if (!input_dev) {
> > >     144                 dev_err(&pdev->dev, "failed to allocate the input device\n");
> > >     145                 return -ENOMEM;
> > >     146         }
> > >     147
> > >     148         input_dev->name = pdev->name;
> > >     149         input_dev->phys = "keyscan-keys/input0";
> > >     150         input_dev->dev.parent = &pdev->dev;
> > >     151         input_dev->open = keyscan_open;
> > >     152         input_dev->close = keyscan_close;
> > >     153
> > >     154         input_dev->id.bustype = BUS_HOST;
> > >     155
> > > --> 156         error = keypad_matrix_key_parse_dt(keypad_data);
> > >                                                    ^^^^^^^^^^^
> > I agree with you this would be a problem
> > to clarify the NULL derefence would occur here within keypad_matrix_key_parse_dt
> >
> > struct device *dev = keypad_data->input_dev->dev.parent;
> >
> > > This assumes we have set "keypad_data->input_dev = input_dev;" but we
> > > don't do that until...
> > >
> > >     157         if (error)
> > >     158                 return error;
> > >     159
> > >     160         error = matrix_keypad_build_keymap(NULL, NULL,
> > >     161                                            keypad_data->n_rows,
> > >     162                                            keypad_data->n_cols,
> > >     163                                            NULL, input_dev);
> > >     164         if (error) {
> > >     165                 dev_err(&pdev->dev, "failed to build keymap\n");
> > >     166                 return error;
> > >     167         }
> > >     168
> > >     169         input_set_drvdata(input_dev, keypad_data);
> > >     170
> > >     171         keypad_data->input_dev = input_dev;
> > >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >
> > > this line here.  This driver has never worked and it was included almost
> > > five years ago.  Is it worth fixing?
> > >
> > >     172
> > >     173         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >     174         keypad_data->base = devm_ioremap_resource(&pdev->dev, res);
> > >     175         if (IS_ERR(keypad_data->base))
> > >     176                 return PTR_ERR(keypad_data->base);
> > >     177
> > >
> > > regards,
> > > dan carpenter
> > >
> >
> > Here is the interesting thing, I was looking on patchwork, and several
> > of the patches including what appears to be the latest actually set
> > "keypad_data->input_dev = input_dev" before calling
> > "keypad_matrix_key_parse_dt"
> >
> > From v4 on patchwork
> > + if (IS_ERR(keypad_data->clk)) {
> > + dev_err(&pdev->dev, "cannot get clock");
> > + return PTR_ERR(keypad_data->clk);
> > + }
> > +
> > + keypad_data->input_dev = input_dev;
> > +
> > + input_dev->name = pdev->name;
> > + input_dev->phys = "keyscan-keys/input0";
> > + input_dev->dev.parent = &pdev->dev;
> > + input_dev->open = keyscan_open;
> > + input_dev->close = keyscan_close;
> > +
> > + input_dev->id.bustype = BUS_HOST;
> > +
> > + error = keypad_matrix_key_parse_dt(keypad_data);
> >
> > According to patchwork, these aren't listed as accepted, so I'm not
> > sure where the exact accepted patch came from. Looking at the commit
> > log, it looks like the issue you showed above was made in the original
> > commit 062589b1399176a9c14bc68e16169f40439d658c so I'm not quite sure
> > what is going on here. Maybe the maintainer can chime in with the
> > original patch/mailing list discussion on this. For reference, I've
> > added the patchwork links below
> > https://patchwork.kernel.org/patch/3854341/
> > https://patchwork.kernel.org/patch/3968891/
> > https://patchwork.kernel.org/patch/3969991/
>
> It may very well be that I messed up when applying the patch. I guess
> whatever platform that is using the driver has not attempted to update
> their kernel since then.
>
> Thanks.
>
> --
> Dmitry

Hi Dmitry,

Thanks for the quick response. Yes I was just looking at the other
mailing lists patchwork and while comments were missing on the
linux-input list for the v4 patch, there was a discussion on the
regular kernel mailing list:
https://lore.kernel.org/patchwork/patch/455450/#630445

It looks like you told him he didn't need to submit a v5 but generated
one based on the changes suggested in the discussion:

[begin quote]
On Wed, Apr 16, 2014 at 10:49:29AM +0200, Gabriel Fernandez wrote:
> On 13 April 2014 07:10, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >
> > Does the version of the patch below still work for you?
> >
> Yes it's was tested on b2000 and b2089 sti boards.
>
> > Thanks.
> >
> > --
> > Dmitry
> >
> Thanks for yours remarks, i will prepare a v5 versions.


If the version I sent to you works then you do not need to prepare v5,
I'll just apply what I have.

Thanks!
[end quote]

So I guess Dan's original question remains, should this be fixed
considering it's so old and apparently nobody could possibly be using
it in the kernel since it doesn't work (in which case the driver
should probably be dropped) or should we fix it?

Thanks,
Ken Sloat