linux-kernel - Re: [PATCH RESEND 1/5] staging: sm750fb: Use memset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAA5enKZqRPx8a9Kk7cX1GJih9=N+Wj_zPbhg+hbLDJoKNEau-Q@mail.gmail.com>
Date:	Wed, 18 Mar 2015 10:44:53 +0000
From:	Lorenzo Stoakes <lstoakes@...il.com>
To:	Dan Carpenter <dan.carpenter@...cle.com>
Cc:	devel@...verdev.osuosl.org, linux-fbdev@...r.kernel.org,
	Teddy Wang <teddy.wang@...iconmotion.com>,
	Greg KH <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org,
	Sudip Mukherjee <sudipm.mukherjee@...il.com>
Subject: Re: [PATCH RESEND 1/5] staging: sm750fb: Use memset_io instead of memset

On 18 March 2015 at 10:18, Dan Carpenter <dan.carpenter@...cle.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/