Thread (14 messages) 14 messages, 2 authors, 2015-03-18

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

From: Lorenzo Stoakes <hidden>
Date: 2015-03-18 10:45:17
Also in: lkml

On 18 March 2015 at 10:18, Dan Carpenter [off-list ref] wrote:
quoted
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help