Re: [RESEND 3/4] vme: change bus error handling scheme

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> On 18 Sep 2015, at 05:46, Martyn Welch <martyn@xxxxxxxxxxxx> wrote:
> 
> 
> 
> On 18/09/15 00:01, dmitry.kalinkin@xxxxxxxxx wrote:
>> From: Dmitry Kalinkin <dmitry.kalinkin@xxxxxxxxx>
>> 
>> The current VME bus error handler adds errors to the bridge error list.
>> vme_master_{read,write} then traverses that list to look for relevant
>> errors.
>> 
>> Such scheme didn't work well for accesses going through vme_master_mmap
>> because they would also allocate a vme_bus_error, but have no way to do
>> vme_clear_errors call to free that memory.
>> 
>> This changes the error handling process to be other way around: now
>> vme_master_{read,write} defines a window in VME address space that will
>> catch possible errors.  VME bus error interrupt only traverses these
>> windows and marks those that had errors in them.
>> 
>> Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@xxxxxxxxx>
>> Cc: Igor Alekseev <igor.alekseev@xxxxxxx>
> 
> This one is currently causing the following warnings at build time:
> 
>  CC      drivers/vme/bridges/vme_tsi148.o
> drivers/vme/bridges/vme_tsi148.c: In function ‘tsi148_master_read’:
> drivers/vme/bridges/vme_tsi148.c:1254:14: warning: ‘handler’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   if (handler->num_errors) {
>              ^
> drivers/vme/bridges/vme_tsi148.c: In function ‘tsi148_master_write’:
> drivers/vme/bridges/vme_tsi148.c:1352:14: warning: ‘handler’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   if (handler->num_errors) {
>              ^
I can’t reproduce this warning on gcc version 4.7.2. And now that I think about it, gcc would not give this warning because of the possible race condition. It will assume that err_chk stays constant.
> 
> This appears to be because the pointer "handler" is set and checked in one if statement controlled by "err_chk" and is used in a second. The variable "err_chk" is a kernel param and from memory this can be set at runtime, so there's a chance that the value of err_chk could be changed between the two if statements (unlikely I know, but...) resulting in handler being a null pointer.
> 
> Martyn
> 
>> ---
>>  drivers/vme/bridges/vme_ca91cx42.c |    3 +-
>>  drivers/vme/bridges/vme_tsi148.c   |   83 ++++++++++++++++----------------
>>  drivers/vme/vme.c                  |   92 +++++++++++++-----------------------
>>  drivers/vme/vme_bridge.h           |   23 +++++----
>>  4 files changed, 91 insertions(+), 110 deletions(-)
>> 
>> diff --git a/drivers/vme/bridges/vme_ca91cx42.c b/drivers/vme/bridges/vme_ca91cx42.c
>> index f692efc..834883d 100644
>> --- a/drivers/vme/bridges/vme_ca91cx42.c
>> +++ b/drivers/vme/bridges/vme_ca91cx42.c
>> @@ -204,8 +204,7 @@ static int ca91cx42_irq_init(struct vme_bridge *ca91cx42_bridge)
>>  	/* Need pdev */
>>  	pdev = container_of(ca91cx42_bridge->parent, struct pci_dev, dev);
>>  -	/* Initialise list for VME bus errors */
>> -	INIT_LIST_HEAD(&ca91cx42_bridge->vme_errors);
>> +	INIT_LIST_HEAD(&ca91cx42_bridge->vme_error_handlers);
>>    	mutex_init(&ca91cx42_bridge->irq_mtx);
>>  diff --git a/drivers/vme/bridges/vme_tsi148.c b/drivers/vme/bridges/vme_tsi148.c
>> index 76ccfae..b0132e0 100644
>> --- a/drivers/vme/bridges/vme_tsi148.c
>> +++ b/drivers/vme/bridges/vme_tsi148.c
>> @@ -314,8 +314,7 @@ static int tsi148_irq_init(struct vme_bridge *tsi148_bridge)
>>    	bridge = tsi148_bridge->driver_priv;
>>  -	/* Initialise list for VME bus errors */
>> -	INIT_LIST_HEAD(&tsi148_bridge->vme_errors);
>> +	INIT_LIST_HEAD(&tsi148_bridge->vme_error_handlers);
>>    	mutex_init(&tsi148_bridge->irq_mtx);
>>  @@ -1187,7 +1186,7 @@ static ssize_t tsi148_master_read(struct vme_master_resource *image, void *buf,
>>  	int retval, enabled;
>>  	unsigned long long vme_base, size;
>>  	u32 aspace, cycle, dwidth;
>> -	struct vme_bus_error *vme_err = NULL;
>> +	struct vme_error_handler *handler;
>>  	struct vme_bridge *tsi148_bridge;
>>  	void __iomem *addr = image->kern_base + offset;
>>  	unsigned int done = 0;
>> @@ -1197,6 +1196,17 @@ static ssize_t tsi148_master_read(struct vme_master_resource *image, void *buf,
>>    	spin_lock(&image->lock);
>>  +	if (err_chk) {
>> +		__tsi148_master_get(image, &enabled, &vme_base, &size, &aspace,
>> +				    &cycle, &dwidth);
>> +		handler = vme_register_error_handler(tsi148_bridge, aspace,
>> +						     vme_base + offset, count);
>> +		if (!handler) {
>> +			spin_unlock(&image->lock);
>> +			return -ENOMEM;
>> +		}
>> +	}
>> +
>>  	/* The following code handles VME address alignment. We cannot use
>>  	 * memcpy_xxx here because it may cut data transfers in to 8-bit
>>  	 * cycles when D16 or D32 cycles are required on the VME bus.
>> @@ -1240,24 +1250,16 @@ static ssize_t tsi148_master_read(struct vme_master_resource *image, void *buf,
>>  out:
>>  	retval = count;
>>  -	if (!err_chk)
>> -		goto skip_chk;
>> -
>> -	__tsi148_master_get(image, &enabled, &vme_base, &size, &aspace, &cycle,
>> -		&dwidth);
>> -
>> -	vme_err = vme_find_error(tsi148_bridge, aspace, vme_base + offset,
>> -		count);
>> -	if (vme_err != NULL) {
>> -		dev_err(image->parent->parent, "First VME read error detected "
>> -			"an at address 0x%llx\n", vme_err->address);
>> -		retval = vme_err->address - (vme_base + offset);
>> -		/* Clear down save errors in this address range */
>> -		vme_clear_errors(tsi148_bridge, aspace, vme_base + offset,
>> -			count);
>> +	if (err_chk) {
>> +		if (handler->num_errors) {
>> +			dev_err(image->parent->parent,
>> +				"First VME read error detected an at address 0x%llx\n",
>> +				handler->first_error);
>> +			retval = handler->first_error - (vme_base + offset);
>> +		}
>> +		vme_unregister_error_handler(handler);
>>  	}
>>  -skip_chk:
>>  	spin_unlock(&image->lock);
>>    	return retval;
>> @@ -1274,7 +1276,7 @@ static ssize_t tsi148_master_write(struct vme_master_resource *image, void *buf,
>>  	unsigned int done = 0;
>>  	unsigned int count32;
>>  -	struct vme_bus_error *vme_err = NULL;
>> +	struct vme_error_handler *handler;
>>  	struct vme_bridge *tsi148_bridge;
>>  	struct tsi148_driver *bridge;
>>  @@ -1284,6 +1286,17 @@ static ssize_t tsi148_master_write(struct vme_master_resource *image, void *buf,
>>    	spin_lock(&image->lock);
>>  +	if (err_chk) {
>> +		__tsi148_master_get(image, &enabled, &vme_base, &size, &aspace,
>> +				    &cycle, &dwidth);
>> +		handler = vme_register_error_handler(tsi148_bridge, aspace,
>> +						     vme_base + offset, count);
>> +		if (!handler) {
>> +			spin_unlock(&image->lock);
>> +			return -ENOMEM;
>> +		}
>> +	}
>> +
>>  	/* Here we apply for the same strategy we do in master_read
>>  	 * function in order to assure the correct cycles.
>>  	 */
>> @@ -1333,30 +1346,18 @@ out:
>>  	 * We check for saved errors in the written address range/space.
>>  	 */
>>  -	if (!err_chk)
>> -		goto skip_chk;
>> -
>> -	/*
>> -	 * Get window info first, to maximise the time that the buffers may
>> -	 * fluch on their own
>> -	 */
>> -	__tsi148_master_get(image, &enabled, &vme_base, &size, &aspace, &cycle,
>> -		&dwidth);
>> -
>> -	ioread16(bridge->flush_image->kern_base + 0x7F000);
>> +	if (err_chk) {
>> +		ioread16(bridge->flush_image->kern_base + 0x7F000);
>>  -	vme_err = vme_find_error(tsi148_bridge, aspace, vme_base + offset,
>> -		count);
>> -	if (vme_err != NULL) {
>> -		dev_warn(tsi148_bridge->parent, "First VME write error detected"
>> -			" an at address 0x%llx\n", vme_err->address);
>> -		retval = vme_err->address - (vme_base + offset);
>> -		/* Clear down save errors in this address range */
>> -		vme_clear_errors(tsi148_bridge, aspace, vme_base + offset,
>> -			count);
>> +		if (handler->num_errors) {
>> +			dev_warn(tsi148_bridge->parent,
>> +				 "First VME write error detected an at address 0x%llx\n",
>> +				 handler->first_error);
>> +			retval = handler->first_error - (vme_base + offset);
>> +		}
>> +		vme_unregister_error_handler(handler);
>>  	}
>>  -skip_chk:
>>  	spin_unlock(&image->lock);
>>    	return retval;
>> diff --git a/drivers/vme/vme.c b/drivers/vme/vme.c
>> index 2b79cd2..7a10d92 100644
>> --- a/drivers/vme/vme.c
>> +++ b/drivers/vme/vme.c
>> @@ -1026,76 +1026,52 @@ EXPORT_SYMBOL(vme_dma_free);
>>  void vme_bus_error_handler(struct vme_bridge *bridge,
>>  			   unsigned long long address, int am)
>>  {
>> -	struct vme_bus_error *error;
>> -
>> -	error = kmalloc(sizeof(struct vme_bus_error), GFP_ATOMIC);
>> -	if (error) {
>> -		error->aspace = vme_get_aspace(am);
>> -		error->address = address;
>> -		list_add_tail(&error->list, &bridge->vme_errors);
>> -	} else {
>> -		dev_err(bridge->parent,
>> -			"Unable to alloc memory for VMEbus Error reporting\n");
>> +	struct list_head *handler_pos = NULL;
>> +	struct vme_error_handler *handler;
>> +	u32 aspace = vme_get_aspace(am);
>> +
>> +	list_for_each(handler_pos, &bridge->vme_error_handlers) {
>> +		handler = list_entry(handler_pos, struct vme_error_handler,
>> +				     list);
>> +		if ((aspace == handler->aspace) &&
>> +		    (address >= handler->start) &&
>> +		    (address < handler->end)) {
>> +			if (!handler->num_errors)
>> +				handler->first_error = address;
>> +			if (handler->num_errors != UINT_MAX)
>> +				handler->num_errors++;
>> +		}
>>  	}
>>  }
>>  EXPORT_SYMBOL(vme_bus_error_handler);
>>  -/*
>> - * Find the first error in this address range
>> - */
>> -struct vme_bus_error *vme_find_error(struct vme_bridge *bridge, u32 aspace,
>> -				     unsigned long long address, size_t count)
>> +struct vme_error_handler *vme_register_error_handler(
>> +	struct vme_bridge *bridge, u32 aspace,
>> +	unsigned long long address, size_t len)
>>  {
>> -	struct list_head *err_pos;
>> -	struct vme_bus_error *vme_err, *valid = NULL;
>> -	unsigned long long bound;
>> -
>> -	bound = address + count;
>> +	struct vme_error_handler *handler;
>>  -	err_pos = NULL;
>> -	/* Iterate through errors */
>> -	list_for_each(err_pos, &bridge->vme_errors) {
>> -		vme_err = list_entry(err_pos, struct vme_bus_error, list);
>> -		if ((vme_err->aspace == aspace) &&
>> -		    (vme_err->address >= address) &&
>> -		    (vme_err->address < bound)) {
>> +	handler = kmalloc(sizeof(*handler), GFP_KERNEL);
>> +	if (!handler)
>> +		return NULL;
>>  -			valid = vme_err;
>> -			break;
>> -		}
>> -	}
>> +	handler->aspace = aspace;
>> +	handler->start = address;
>> +	handler->end = address + len;
>> +	handler->num_errors = 0;
>> +	handler->first_error = 0;
>> +	list_add_tail(&handler->list, &bridge->vme_error_handlers);
>>  -	return valid;
>> +	return handler;
>>  }
>> -EXPORT_SYMBOL(vme_find_error);
>> +EXPORT_SYMBOL(vme_register_error_handler);
>>  -/*
>> - * Clear errors in the provided address range.
>> - */
>> -void vme_clear_errors(struct vme_bridge *bridge, u32 aspace,
>> -		      unsigned long long address, size_t count)
>> +void vme_unregister_error_handler(struct vme_error_handler *handler)
>>  {
>> -	struct list_head *err_pos, *temp;
>> -	struct vme_bus_error *vme_err;
>> -	unsigned long long bound;
>> -
>> -	bound = address + count;
>> -
>> -	err_pos = NULL;
>> -	/* Iterate through errors */
>> -	list_for_each_safe(err_pos, temp, &bridge->vme_errors) {
>> -		vme_err = list_entry(err_pos, struct vme_bus_error, list);
>> -
>> -		if ((vme_err->aspace == aspace) &&
>> -		    (vme_err->address >= address) &&
>> -		    (vme_err->address < bound)) {
>> -
>> -			list_del(err_pos);
>> -			kfree(vme_err);
>> -		}
>> -	}
>> +	list_del(&handler->list);
>> +	kfree(handler);
>>  }
>> -EXPORT_SYMBOL(vme_clear_errors);
>> +EXPORT_SYMBOL(vme_unregister_error_handler);
>>    void vme_irq_handler(struct vme_bridge *bridge, int level, int statid)
>>  {
>> diff --git a/drivers/vme/vme_bridge.h b/drivers/vme/vme_bridge.h
>> index 92fbe18..397578a 100644
>> --- a/drivers/vme/vme_bridge.h
>> +++ b/drivers/vme/vme_bridge.h
>> @@ -75,10 +75,13 @@ struct vme_lm_resource {
>>  	int monitors;
>>  };
>>  -struct vme_bus_error {
>> +struct vme_error_handler {
>>  	struct list_head list;
>> -	u32 aspace;
>> -	unsigned long long address;
>> +	unsigned long long start;	/* Beginning of error window */
>> +	unsigned long long end;		/* End of error window */
>> +	unsigned long long first_error;	/* Address of the first error */
>> +	u32 aspace;			/* Address space of error window*/
>> +	unsigned num_errors;		/* Number of errors */
>>  };
>>    struct vme_callback {
>> @@ -106,8 +109,10 @@ struct vme_bridge {
>>  	struct list_head dma_resources;
>>  	struct list_head lm_resources;
>>  -	struct list_head vme_errors;	/* List for errors generated on VME */
>> -	struct list_head devices;	/* List of devices on this bridge */
>> +	/* List for registered errors handlers */
>> +	struct list_head vme_error_handlers;
>> +	/* List of devices on this bridge */
>> +	struct list_head devices;
>>    	/* Bridge Info - XXX Move to private structure? */
>>  	struct device *parent;	/* Parent device (eg. pdev->dev for PCI) */
>> @@ -168,13 +173,13 @@ struct vme_bridge {
>>    void vme_bus_error_handler(struct vme_bridge *bridge,
>>  			   unsigned long long address, int am);
>> -struct vme_bus_error *vme_find_error(struct vme_bridge *bridge, u32 aspace,
>> -				     unsigned long long address, size_t count);
>> -void vme_clear_errors(struct vme_bridge *bridge, u32 aspace,
>> -		      unsigned long long address, size_t count);
>>  void vme_irq_handler(struct vme_bridge *, int, int);
>>    int vme_register_bridge(struct vme_bridge *);
>>  void vme_unregister_bridge(struct vme_bridge *);
>> +struct vme_error_handler *vme_register_error_handler(
>> +	struct vme_bridge *bridge, u32 aspace,
>> +	unsigned long long address, size_t len);
>> +void vme_unregister_error_handler(struct vme_error_handler *handler);
>>    #endif /* _VME_BRIDGE_H_ */
> 

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux