linux-kernel - Re: [PATCH 2/6] staging: vme: make [alloc|free]

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:	Wed, 10 Aug 2011 14:34:03 +0100
From:	Martyn Welch <martyn.welch@...com>
To:	Joe Perches <joe@...ches.com>
CC:	Manohar Vanga <manohar.vanga@...n.ch>, gregkh@...e.de,
	cota@...ap.org, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/6] staging: vme: make [alloc|free]_consistent bridge
 specific

On 10/08/11 14:12, Joe Perches wrote:
> On Wed, 2011-08-10 at 11:33 +0200, Manohar Vanga wrote:
>> Make PCI dependent functions ([alloc|free]_consistent() in
>> 'vme.c') bridge specific. By removing the dependency of the
>> VME bridge framework on PCI, this patch allows for addition of
>> non-PCI based VME bridges.
> []
>> diff --git a/drivers/staging/vme/bridges/vme_ca91cx42.c b/drivers/staging/vme/bridges/vme_ca91cx42.c
> []
>> +void *ca91cx42_alloc_consistent(struct device *parent, size_t size,
>> +	dma_addr_t *dma)
>> +{
>> +	struct pci_dev *pdev;
>> +
>> +	/* Find pci_dev container of dev */
>> +	pdev = container_of(parent, struct pci_dev, dev);
>> +
>> +	return pci_alloc_consistent(pdev, size, dma);
>> +}
>> +
>> +void ca91cx42_free_consistent(struct device *parent, size_t size, void *vaddr,
>> +	dma_addr_t dma)
>> +{
>> +	struct pci_dev *pdev;
>> +
>> +	/* Find pci_dev container of dev */
>> +	pdev = container_of(parent, struct pci_dev, dev);
>> +
>> +	pci_free_consistent(pdev, size, vaddr, dma);
>> +}
> []
>> diff --git a/drivers/staging/vme/bridges/vme_tsi148.c b/drivers/staging/vme/bridges/vme_tsi148.c
> []
>> @@ -2122,6 +2122,28 @@ static int tsi148_slot_get(struct vme_bridge *tsi148_bridge)
>>  	return (int)slot;
>>  }
>>  
>> +void *tsi148_alloc_consistent(struct device *parent, size_t size,
>> +	dma_addr_t *dma)
>> +{
>> +	struct pci_dev *pdev;
>> +
>> +	/* Find pci_dev container of dev */
>> +	pdev = container_of(parent, struct pci_dev, dev);
>> +
>> +	return pci_alloc_consistent(pdev, size, dma);
>> +}
>> +
>> +void tsi148_free_consistent(struct device *parent, size_t size, void *vaddr,
>> +	dma_addr_t dma)
>> +{
>> +	struct pci_dev *pdev;
>> +
>> +	/* Find pci_dev container of dev */
>> +	pdev = container_of(parent, struct pci_dev, dev);
>> +
>> +	pci_free_consistent(pdev, size, vaddr, dma);
>> +}
>> +
>>  static int __init tsi148_init(void)
>>  {
>>  	return pci_register_driver(&tsi148_driver);
> 
> Except for the name, those 2 blocks are identical.
> Maybe create a non-pci generic version instead?
> 

I'm not sure you can (I spent quite a bit of time attempting to do just that
when I wrote the original). It just so happens that both of the bridges we
have at the moment are PCI devices, so this does end up looking like copying
common code into the bridges, but assuming a bridge driver comes along that
doesn't use PCI, having this code per driver makes much more sense.


>> diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
> []
>> @@ -104,28 +100,29 @@ void *vme_alloc_consistent(struct vme_resource *resource, size_t size,
>>  		return NULL;
>>  	}
>>  
>> -	/* Find pci_dev container of dev */
>>  	if (bridge->parent == NULL) {
>> -		printk(KERN_ERR "Dev entry NULL\n");
>> +		printk(KERN_ERR "Dev entry NULL for"
>> +			" bridge %s\n", bridge->name);
> 
> Please don't split formats like this.
> Use unsplit format strings even if they exceed 80 cols.
> This one doesn't.
> 
> 		printk(KERN_ERR "Dev entry NULL for bridge %s\n", bridge->name);
> or
> 		pr_err("Dev entry NULL for bridge %s\n", bridge->name);
> 

True, didn't think about that.

Martyn

>> +		return NULL;
>> +	}
>> +
>> +	if (bridge->alloc_consistent == NULL) {
>> +		printk(KERN_ERR "alloc_consistent not supported by"
>> +			" bridge %s\n", bridge->name);
> 
> 		printk(KERN_ERR "alloc_consistent not supported by bridge %s\n",
> 		       bridge->name);
> 
>> @@ -138,10 +135,19 @@ void vme_free_consistent(struct vme_resource *resource, size_t size,
>>  		return;
>>  	}
>>  
>> -	/* Find pci_dev container of dev */
>> -	pdev = container_of(bridge->parent, struct pci_dev, dev);
>> +	if (bridge->parent == NULL) {
>> +		printk(KERN_ERR "Dev entry NULL for"
>> +			" bridge %s\n", bridge->name);
> 
> etc.
> 
> 


-- 
Martyn Welch (Principal Software Engineer) | Registered in England and
GE Intelligent Platforms                   | Wales (3828642) at 100
T +44(0)127322748                          | Barbirolli Square, Manchester,
E martyn.welch@...com                      | M2 3AB  VAT:GB 927559189
--
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/