Re: [PATCH RESEND 1/5] staging: sm750fb: Use memset_io instead of memset

[Lorenzo Stoakes]

On 18 March 2015 at 10:18, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>>
>> This changelog still sucks.  It doesn't describe the effect of this
>> behavior change for the user.  It doesn't even make it clear that you
>> are aware that this is a behavior change.
>
> It doesn't say to me that you have asked yourself if the sparse
> annotations are correct.  Many times they are wrong.

My understanding, which as a new contributor is of course limited and
likely simply wrong in many aspects, is - these memset's are referring
to I/O mapped memory, which as far as I can tell is actually the case
here, so in order to make it explicit that this is the case and we
know it is, we use memset_io. As far as I understand the pointers
simply have a modifier applied which marks them as I/O mapped memory
for the purposes of sparse checking whether they are used consistently
as such and are not treated like they are a normal kernel pointer.

In this case the cursor->vstart and crtc->vScreen pointers, looking
through the source, explicitly refer to memory which is I/O mapped,
and is annotated as __iomem accordingly throughout.

I will update the message accordingly, obviously if I'm
misunderstanding something let me know.

> We have had this discussion before but you still sent the same exact
> bad changelog.

Actually you said:-

> When I see a patch like this, then I worry, "What if the Sparse
> annotations are wrong?  The patch description doesn't say anything about
> that."  After review then I think the annotations are correct so that's
> fine.

And:-

> Yes.  The patch is correct.  I wasn't asking you to redo it.  From later
> patches it's actually clear that you know that this change is a bugfix
> and a behavior change.  But we get a lot of patches where people just
> randomly change things to please Sparse and it maybe silences a warning
> but it's not correct.  I can think of a few recentish examples where
> people used standard struct types which hold __iomem or __user pointers
> but they used them in non-standard ways so the pointers were actually
> normal kernel pointers.

So it wasn't clear *to me* you wanted me to change that, given you
asked me *not* to redo it explicitly (which I assumed applied to the
message too) - apologies if I misinterpreted this!

Best,

-- 
Lorenzo Stoakes
https:/ljs.io
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/