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

Lorenzo Stoakes
2015-03-18T10:45:17Z

On 18 March 2015 at 10:18, Dan Carpenter <dan.carpenter@oracle.com> 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