Re: [PATCH v2 3/8] s390/cio: add basic protected virtualization support

From: Michael Mueller <mimu@linux.ibm.com>
To: Sebastian Ott <sebott@linux.ibm.com>
Cc: KVM Mailing List <kvm@vger.kernel.org>,
	Linux-S390 Mailing List <linux-s390@vger.kernel.org>,
	Cornelia Huck <cohuck@redhat.com>,
	Heiko Carstens <heiko.carstens@de.ibm.com>,
	Halil Pasic <pasic@linux.ibm.com>,
	virtualization@lists.linux-foundation.org,
	"Michael S . Tsirkin" <mst@redhat.com>,
	Christoph Hellwig <hch@infradead.org>,
	Thomas Huth <thuth@redhat.com>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Viktor Mihajlovski <mihajlov@linux.ibm.com>,
	Vasily Gorbik <gor@linux.ibm.com>,
	Janosch Frank <frankja@linux.ibm.com>,
	Claudio Imbrenda <imbrenda@linux.ibm.com>,
	Farhan Ali <alifm@linux.ibm.com>,
	Eric Farman <farman@linux.ibm.com>,
	Pierre Morel <pmorel@linux.ibm.com>
Subject: Re: [PATCH v2 3/8] s390/cio: add basic protected virtualization support
Date: Mon, 27 May 2019 17:01:12 +0200	[thread overview]
Message-ID: <f2b8d5c3-a39b-8632-c463-cde47bf38c91@linux.ibm.com> (raw)
In-Reply-To: <alpine.LFD.2.21.1905251124230.3359@schleppi>

On 25.05.19 11:44, Sebastian Ott wrote:
> 
> On Thu, 23 May 2019, Michael Mueller wrote:
>>   static struct ccw_device * io_subchannel_allocate_dev(struct subchannel *sch)
>>   {
>>   	struct ccw_device *cdev;
>> +	struct gen_pool *dma_pool;
>>   
>>   	cdev  = kzalloc(sizeof(*cdev), GFP_KERNEL);
>> -	if (cdev) {
>> -		cdev->private = kzalloc(sizeof(struct ccw_device_private),
>> -					GFP_KERNEL | GFP_DMA);
>> -		if (cdev->private)
>> -			return cdev;
>> -	}
>> +	if (!cdev)
>> +		goto err_cdev;
>> +	cdev->private = kzalloc(sizeof(struct ccw_device_private),
>> +				GFP_KERNEL | GFP_DMA);
>> +	if (!cdev->private)
>> +		goto err_priv;
>> +	cdev->dev.coherent_dma_mask = sch->dev.coherent_dma_mask;
>> +	cdev->dev.dma_mask = &cdev->dev.coherent_dma_mask;
>> +	dma_pool = cio_gp_dma_create(&cdev->dev, 1);
> 
> This can return NULL. gen_pool_alloc will panic in this case.
> [...]

yep, will handled in next version

> 
>> +err_dma_area:
>> +		kfree(io_priv);

one tab gone

> 
> Indentation.
> 
>> +err_priv:
>> +	put_device(&sch->dev);
>> +	return ERR_PTR(-ENOMEM);
>>   }
> [...]
>>   void ccw_device_update_sense_data(struct ccw_device *cdev)
>>   {
>>   	memset(&cdev->id, 0, sizeof(cdev->id));
>> -	cdev->id.cu_type   = cdev->private->senseid.cu_type;
>> -	cdev->id.cu_model  = cdev->private->senseid.cu_model;
>> -	cdev->id.dev_type  = cdev->private->senseid.dev_type;
>> -	cdev->id.dev_model = cdev->private->senseid.dev_model;
>> +	cdev->id.cu_type   =
>> +		cdev->private->dma_area->senseid.cu_type;
>> +	cdev->id.cu_model  =
>> +		cdev->private->dma_area->senseid.cu_model;
>> +	cdev->id.dev_type  =
>> +		cdev->private->dma_area->senseid.dev_type;
>> +	cdev->id.dev_model =
>> +		cdev->private->dma_area->senseid.dev_model;
> 
> These fit into one line.

yep, surprisingly below 80 characters

> 
>> +/**
>> + * Allocate zeroed dma coherent 31 bit addressable memory using
>> + * the subchannels dma pool. Maximal size of allocation supported
>> + * is PAGE_SIZE.
>> + */
> drivers/s390/cio/device_ops.c:708: warning: Function parameter or member 'cdev' not described in 'ccw_device_dma_zalloc'
> drivers/s390/cio/device_ops.c:708: warning: Function parameter or member 'size' not described in 'ccw_device_dma_zalloc'

changing comment open token

> 
> 
> Reviewed-by: Sebastian Ott <sebott@linux.ibm.com>
> 

Thanks!

Michael