linux-kernel - Re: staging driver s626 clashes with philips SAA7146 media/dvb based cards

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:	Thu, 18 Jun 2009 21:43:07 +0400
From:	Manu Abraham <abraham.manu@...il.com>
To:	Herton Ronaldo Krzesinski <herton@...driva.com.br>
Cc:	fmhess@...rs.sourceforge.net, Ian Abbott <abbotti@....co.uk>,
	Greg KH <gregkh@...e.de>, LKML <linux-kernel@...r.kernel.org>,
	Gianluca Palli <gpalli@...s.unibo.it>,
	David Schleef <ds@...leef.org>
Subject: Re: staging driver s626 clashes with philips SAA7146 media/dvb based 
	cards

Hi Herton,

On Thu, Jun 18, 2009 at 9:23 PM, Herton Ronaldo
Krzesinski<herton@...driva.com.br> wrote:
> Em Quinta-feira 18 Junho 2009, às 04:28:04, Frank Mori Hess escreveu:
>> On Wednesday 17 June 2009, Herton Ronaldo Krzesinski wrote:
>> > >
>> > > Your patch breaks configuration of the board unless the bus and slot
>> > > are explicitly specified.  Just make a minimal patch that replaces
>> > > pci_get_device with pci_get_subsys and fixes the problem that was
>> > > reported.
>> >
>> > Hmm that's not what the patch does, it doesn't break configuration,
>> > keeps the same logic as before (I was wrong in my last email replying to
>> > myself), check it, if it->options[0] and it->options[1] isn't specified,
>> > the pdev is valid so the for loop exits (see !pdev check).
>>
>> Your right.  However, it also turns a loop over pci devices into a loop
>> over pci ids, which appears to break the case of multiple s626 boards
>> where the bus/slot of the second s626 board is specified.  If you're not
>> willing to provide a minimal patch that just fixes the reported problem,
>> just say so.  It would have been less effort for me to do it myself than
>> analyze what your changes are breaking.
>
> That's part of reviewing process, I just wanted to enhance it in case more pci
> ids are added in the future along with the switch to pci_get_subsys, I don't
> know why you act like that, you don't want the code to be enhanced? Other
> comedi driver loop over ids, for example gsc_hpdi
>
> And indeed what you say it's true, there is a bug in the patch now that you
> checked it out properly, it has a problem with this multiple s626 case, fixed
> that now. Pointing out the problem instead just saying it's broken helps :)
>
> I also removed the obligation to add sub ids and re-factored the patch, that
> was too much and not good, just a simpler loop over pci id array is used now,
> and a comment was added in pci id list telling people that they should add
> subvendor:subdevice ids for boards with vendor:device == 0x1131:0x7146,
> in case new boards with this id appear.


Since there is valid subsystem information: why do you have to loop
over the ID's
manually in the driver ?

Can't the ID's be set into the PCI ID table and a normal pci_probe be
used instead ?
Not that i am familiar with the comedi stuff though ...

We use the probe method directly (rather than pci_get_ ) for most of
the multimedia drivers. As an example this is what i do:
http://jusst.de/hg/saa716x/file/59dd985d4473/linux/drivers/media/dvb/saa716x/saa716x_budget.c

Maybe it helps you in some way.

Regards,
Manu
--
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/