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]

Message-ID: <20100501090827.72e9da71@bike.lwn.net>
Date:	Sat, 1 May 2010 09:08:27 -0600
From:	Jonathan Corbet <corbet@....net>
To:	Florian Tobias Schandinat <FlorianSchandinat@....de>
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

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.

> > @@ -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?

Thanks,

jon
--
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/