On Fri, Feb 15, 2019 at 07:51:56PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 15, 2019 at 4:40 PM Heikki Krogerus
> <[email protected]> wrote:
> >
> > On Fri, Feb 15, 2019 at 11:19:47AM +0200, Andy Shevchenko wrote:
> > > On Fri, Feb 15, 2019 at 11:01 AM Heikki Krogerus
> > > <[email protected]> wrote:
> > > >
> > > > On Fri, Feb 15, 2019 at 01:33:24PM +0800, kbuild test robot wrote:
> > > > > tree:
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
> > > > > usb-testing
> > > > > head: 09aa11cfda9d8186046bcd1adcd6498b688114f4
> > > > > commit: 96a6d031ca9930938bd66d0052fc7ed2b56e3583 [63/64] usb: typec:
> > > > > mux: Find the muxes by also matching against the device node
> > > > >
> > > > > smatch warnings:
> > > > > drivers/usb/typec/mux.c:167 typec_mux_match() warn: unsigned 'nval'
> > > > > is never less than zero.
> > > >
> > > > I'm assuming this means we should not use type 'size_t' in this case.
> > > >
> > > > I'll send a patch where I'll use 'int' instead if 'size_t'.
> > >
> > > Perhaps, in case of < 0 it should return it as error pointer to the
> > > caller?
> >
> > It already does that.
>
> Not in a line 159/160 AFAICS.
It get's tricky. We would then have to at least check that the error
value is not -ENODATA or -EINVAL, in those cases we should still
return NULL, but we can of course do that... Actually, now that I
think about it, we should return NULL also in case of -ENXIO. Even
that means we ignore the case.
I don't see a possible error path where we should return the error to
the caller when checking the number of elements.
> > > > > vim +/nval +167 drivers/usb/typec/mux.c
> > > > >
> > > > > 124
> > > > > 125 static void *typec_mux_match(struct device_connection
> > > > > *con, int ep, void *data)
> > > > > 126 {
> > > > > 127 const struct typec_altmode_desc *desc = data;
> > > > > 128 struct typec_mux *mux;
> > > > > 129 size_t nval;
> > > > > 130 bool match;
> > > > > 131 u16 *val;
> > > > > 132 int i;
> > > > > 133
> > > > > 134 if (!con->fwnode) {
> > > > > 135 list_for_each_entry(mux, &mux_list,
> > > > > entry)
> > > > > 136 if (!strcmp(con->endpoint[ep],
> > > > > dev_name(mux->dev)))
> > > > > 137 return mux;
> > > > > 138 return ERR_PTR(-EPROBE_DEFER);
> > > > > 139 }
> > > > > 140
> > > > > 141 /*
> > > > > 142 * Check has the identifier already been
> > > > > "consumed". If it
> > > > > 143 * has, no need to do any extra connection
> > > > > identification.
> > > > > 144 */
> > > > > 145 match = !con->id;
> > > > > 146 if (match)
> > > > > 147 goto find_mux;
> > > > > 148
> > > > > 149 /* Accessory Mode muxes */
> > > > > 150 if (!desc) {
> > > > > 151 match =
> > > > > fwnode_property_present(con->fwnode, "accessory");
> > > > > 152 if (match)
> > > > > 153 goto find_mux;
> > > > > 154 return NULL;
> > > > > 155 }
> > > > > 156
> > > > > 157 /* Alternate Mode muxes */
> > > > > 158 nval =
> > > > > fwnode_property_read_u16_array(con->fwnode, "svid", NULL, 0);
> > > > > 159 if (nval <= 0)
> > > > > 160 return NULL;
> > > > > 161
> > > > > 162 val = kcalloc(nval, sizeof(*val), GFP_KERNEL);
> > > > > 163 if (!val)
> > > > > 164 return ERR_PTR(-ENOMEM);
> > > > > 165
> > > > > 166 nval =
> > > > > fwnode_property_read_u16_array(con->fwnode, "svid", val, nval);
> > > > > > 167 if (nval < 0) {
> > > > > 168 kfree(val);
> > > > > 169 return ERR_PTR(nval);
> > > > > 170 }
> > > > > 171
> > > > > 172 for (i = 0; i < nval; i++) {
> > > > > 173 match = val[i] == desc->svid;
> > > > > 174 if (match) {
> > > > > 175 kfree(val);
> > > > > 176 goto find_mux;
> > > > > 177 }
> > > > > 178 }
> > > > > 179 kfree(val);
> > > > > 180 return NULL;
> > > > > 181
> > > > > 182 find_mux:
> > > > > 183 list_for_each_entry(mux, &mux_list, entry)
> > > > > 184 if (dev_fwnode(mux->dev) == con->fwnode)
> > > > > 185 return mux;
> > > > > 186
> > > > > 187 return match ? ERR_PTR(-EPROBE_DEFER) : NULL;
> > > > > 188 }
> > > > > 189
thanks,
--
heikki
