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