Re: [Outreachy kernel] Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary

[Julia Lawall <julia.lawall@lip6.fr>]

On Sat, 19 Oct 2019, Jules Irenge wrote:

>
>
> On Sat, 19 Oct 2019, Dan Carpenter wrote:
>
> > On Sat, Oct 19, 2019 at 03:07:15PM +0100, Jules Irenge wrote:
> > > diff --git a/drivers/staging/wfx/bh.c b/drivers/staging/wfx/bh.c
> > > index 3355183fc86c..573216b08042 100644
> > > --- a/drivers/staging/wfx/bh.c
> > > +++ b/drivers/staging/wfx/bh.c
> > > @@ -69,13 +69,13 @@ static int rx_helper(struct wfx_dev *wdev, size_t read_len, int *is_cnf)
> > >  	if (wfx_data_read(wdev, skb->data, alloc_len))
> > >  		goto err;
> > >
> > > -	piggyback = le16_to_cpup((u16 *) (skb->data + alloc_len - 2));
> > > +	piggyback = le16_to_cpup((u16 *)(skb->data + alloc_len - 2));
> > >  	_trace_piggyback(piggyback, false);
> > >
> > > -	hif = (struct hif_msg *) skb->data;
> > > +	hif = (struct hif_msg *)skb->data;
> > >  	WARN(hif->encrypted & 0x1, "unsupported encryption type");
> > >  	if (hif->encrypted == 0x2) {
> > > -		if (wfx_sl_decode(wdev, (void *) hif)) {
> > > +		if (wfx_sl_decode(wdev, (void *)hif)) {
> >
> > In the future you may want to go through and remove the (void *) casts.
> > It's not required here.
> >
> > > diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
> > > index f65f7d75e731..effd07957753 100644
> > > --- a/drivers/staging/wfx/bus_spi.c
> > > +++ b/drivers/staging/wfx/bus_spi.c
> > > @@ -90,7 +90,7 @@ static int wfx_spi_copy_to_io(void *priv, unsigned int addr,
> > >  	struct wfx_spi_priv *bus = priv;
> > >  	u16 regaddr = (addr << 12) | (count / 2);
> > >  	// FIXME: use a bounce buffer
> > > -	u16 *src16 = (void *) src;
> > > +	u16 *src16 = (void *)src;
> >
> > Here we are just getting rid of the constness.  Apparently we are doing
> > that so we can modify it without GCC pointing out the bug!!  I don't
> > know the code but this seems very wrong.
> >
> Checkpatch was complaining about  space between type cast and the
> variable. I just get rid of the space. Well I don't know whether this was
> false positive one.

I think you missed the point.  It would be good to trace through the core
and try to figure out where this src value comes from.  Is it really
const?  Or is the const declaration there just to satisfy the type
checker, and is the actual data provided not const.  This function is
stored in a hwbus_ops structure.  It would be good to see what other
drivers that store a function in the same field of such a structure do,
and to see where the function is actually called (via a function pointer)
and where the argument comes from.

julia