linux-kernel - Re: [PATCH 12/30] viafb: Move core stuff into via-core.c

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]

Date:	Sat, 01 May 2010 17:29:40 +0200
From:	Florian Tobias Schandinat <FlorianSchandinat@....de>
To:	Jonathan Corbet <corbet@....net>
CC:	linux-kernel@...r.kernel.org, Harald Welte <laforge@...monks.org>,
	linux-fbdev@...r.kernel.org, JosephChan@....com.tw,
	ScottFang@...tech.com.cn
Subject: Re: [PATCH 12/30] viafb: Move core stuff into via-core.c

Jonathan Corbet schrieb:
> On Sat, 01 May 2010 17:02:30 +0200
> Florian Tobias Schandinat <FlorianSchandinat@....de> wrote:
> 
>>>  struct fb_info *viafbinfo;
>>> +EXPORT_SYMBOL_GPL(viafbinfo);
>>>  struct fb_info *viafbinfo1;
>>>  struct viafb_par *viaparinfo;
>>> +EXPORT_SYMBOL_GPL(viaparinfo);
>>>  struct viafb_par *viaparinfo1;  
>> Ugh, I really hope you introduce those only as temporary exports until 
>> the split is finished. It's ugly enough that viafb uses these internally 
>> as global variables which will vanish in some time but for a 
>> multifunction driver having those sounds even more ridiculous. If we 
>> agree that it's only a temporary solution I'll take this bitter pill.
> 
> No we don't agree...  what we're seeing here is some history that I did
> not succeed in getting rid of entirely.  Those exports have no reason
> to exist anymore and shouldn't have slipped through into that patch.  I
> will most certainly make them go away.

That's even better.

>>> @@ -1764,6 +1765,7 @@ static int __devinit via_pci_probe(struct pci_dev *pdev,
>>>  		&viaparinfo->shared->lvds_setting_info2;
>>>  	viaparinfo->crt_setting_info = &viaparinfo->shared->crt_setting_info;
>>>  	viaparinfo->chip_info = &viaparinfo->shared->chip_info;
>>> +	spin_lock_init(&viaparinfo->reg_lock);  
>> I think the initialization of the lock that is made for synchronization 
>> of hardware access should be in the via-core.c you just introduce. (and 
>>   the lock itself in a structure or something outside the framebuffer 
>> flow). Just saw that you did so in your next patch, so there is no 
>> reason to needlessly introduce the spinlock now.
> 
> As you note, it's only there for one step in the series, and no
> electrons are harmed in the process.  Is this really worth the trouble
> of changing?

Well I only noticed it just before sending. Not needlessly changing code 
would make the patches simpler but as I know now what is going on I 
don't insist on changing it.


Thanks,

Florian Tobias Schandinat
--
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/