tjh.dev / kernel
Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
fork atom

W1: split master mutex to avoid deadlocks.

The 'mutex' in struct w1_master is use for two very different
purposes.

Firstly it protects various data structures such as the list of all
slaves.

Secondly it protects the w1 buss against concurrent accesses.

This can lead to deadlocks when the ->probe code called while adding a
slave needs to talk on the bus, as is the case for power_supply
devices.
ds2780 and ds2781 drivers contain a work around to track which
process hold the lock simply to avoid this deadlock. bq27000 doesn't
have that work around and so deadlocks.

There are other possible deadlocks involving sysfs.
When removing a device the sysfs s_active lock is held, so the lock
that protects the slave list must take precedence over s_active.
However when access power_supply attributes via sysfs, the s_active
lock must take precedence over the lock that protects accesses to
the bus.

So to avoid deadlocks between w1 slaves and sysfs, these must be
two separate locks. Making them separate means that the work around
in ds2780 and ds2781 can be removed.

So this patch:
- adds a new mutex: "bus_mutex" which serialises access to the bus.
- takes in mutex in w1_search and ds1wm_search while they access
the bus for searching. The mutex is dropped before calling the
callback which adds the slave.
- changes all slaves to use bus_mutex instead of mutex to
protect access to the bus
- removes w1_ds2790_io_nolock and w1_ds2781_io_nolock, and the
related code from drivers/power/ds278[01]_battery.c which
calls them.

Signed-off-by: NeilBrown <neilb@suse.de>
Acked-by: Evgeniy Polyakov <zbr@ioremap.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

authored by

NeilBrown and committed by
Greg Kroah-Hartman b02f8bed b7e938d0

+57 -95
+1 -10
drivers/power/ds2780_battery.c
··· 39 39 struct device *dev; 40 40 struct power_supply bat; 41 41 struct device *w1_dev; 42 - struct task_struct *mutex_holder; 43 42 }; 44 43 45 44 enum current_types { ··· 63 64 static inline int ds2780_battery_io(struct ds2780_device_info *dev_info, 64 65 char *buf, int addr, size_t count, int io) 65 66 { 66 - if (dev_info->mutex_holder == current) 67 - return w1_ds2780_io_nolock(dev_info->w1_dev, buf, addr, count, io); 68 - else 69 - return w1_ds2780_io(dev_info->w1_dev, buf, addr, count, io); 67 + return w1_ds2780_io(dev_info->w1_dev, buf, addr, count, io); 70 68 } 71 69 72 70 static inline int ds2780_read8(struct ds2780_device_info *dev_info, u8 *val, ··· 775 779 dev_info->bat.properties = ds2780_battery_props; 776 780 dev_info->bat.num_properties = ARRAY_SIZE(ds2780_battery_props); 777 781 dev_info->bat.get_property = ds2780_battery_get_property; 778 - dev_info->mutex_holder = current; 779 782 780 783 ret = power_supply_register(&pdev->dev, &dev_info->bat); 781 784 if (ret) { ··· 804 809 goto fail_remove_bin_file; 805 810 } 806 811 807 - dev_info->mutex_holder = NULL; 808 - 809 812 return 0; 810 813 811 814 fail_remove_bin_file: ··· 822 829 static int __devexit ds2780_battery_remove(struct platform_device *pdev) 823 830 { 824 831 struct ds2780_device_info *dev_info = platform_get_drvdata(pdev); 825 - 826 - dev_info->mutex_holder = current; 827 832 828 833 /* remove attributes */ 829 834 sysfs_remove_group(&dev_info->bat.dev->kobj, &ds2780_attr_group);
+1 -11
drivers/power/ds2781_battery.c
··· 37 37 struct device *dev; 38 38 struct power_supply bat; 39 39 struct device *w1_dev; 40 - struct task_struct *mutex_holder; 41 40 }; 42 41 43 42 enum current_types { ··· 61 62 static inline int ds2781_battery_io(struct ds2781_device_info *dev_info, 62 63 char *buf, int addr, size_t count, int io) 63 64 { 64 - if (dev_info->mutex_holder == current) 65 - return w1_ds2781_io_nolock(dev_info->w1_dev, buf, addr, 66 - count, io); 67 - else 68 - return w1_ds2781_io(dev_info->w1_dev, buf, addr, count, io); 65 + return w1_ds2781_io(dev_info->w1_dev, buf, addr, count, io); 69 66 } 70 67 71 68 int w1_ds2781_read(struct ds2781_device_info *dev_info, char *buf, ··· 770 775 dev_info->bat.properties = ds2781_battery_props; 771 776 dev_info->bat.num_properties = ARRAY_SIZE(ds2781_battery_props); 772 777 dev_info->bat.get_property = ds2781_battery_get_property; 773 - dev_info->mutex_holder = current; 774 778 775 779 ret = power_supply_register(&pdev->dev, &dev_info->bat); 776 780 if (ret) { ··· 799 805 goto fail_remove_bin_file; 800 806 } 801 807 802 - dev_info->mutex_holder = NULL; 803 - 804 808 return 0; 805 809 806 810 fail_remove_bin_file: ··· 817 825 static int __devexit ds2781_battery_remove(struct platform_device *pdev) 818 826 { 819 827 struct ds2781_device_info *dev_info = platform_get_drvdata(pdev); 820 - 821 - dev_info->mutex_holder = current; 822 828 823 829 /* remove attributes */ 824 830 sysfs_remove_group(&dev_info->bat.dev->kobj, &ds2781_attr_group);
+4
drivers/w1/masters/ds1wm.c
··· 334 334 return; 335 335 } 336 336 337 + mutex_lock(&master_dev->bus_mutex); 337 338 if (ds1wm_reset(ds1wm_data)) { 339 + mutex_unlock(&master_dev->bus_mutex); 338 340 dev_dbg(&ds1wm_data->pdev->dev, 339 341 "pass: %d reset error (or no slaves)\n", pass); 340 342 break; ··· 389 387 390 388 } 391 389 if (ds1wm_data->read_error) { 390 + mutex_unlock(&master_dev->bus_mutex); 392 391 dev_err(&ds1wm_data->pdev->dev, 393 392 "pass: %d read error, retrying\n", pass); 394 393 break; ··· 403 400 dev_dbg(&ds1wm_data->pdev->dev, 404 401 "pass: %d resetting bus\n", pass); 405 402 ds1wm_reset(ds1wm_data); 403 + mutex_unlock(&master_dev->bus_mutex); 406 404 if ((r_prime & ((u64)1 << 63)) && (d & ((u64)1 << 63))) { 407 405 dev_err(&ds1wm_data->pdev->dev, 408 406 "pass: %d bus error, retrying\n", pass);
+2 -2
drivers/w1/slaves/w1_bq27000.c
··· 31 31 u8 val; 32 32 struct w1_slave *sl = container_of(dev->parent, struct w1_slave, dev); 33 33 34 - mutex_lock(&sl->master->mutex); 34 + mutex_lock(&sl->master->bus_mutex); 35 35 w1_write_8(sl->master, HDQ_CMD_READ | reg); 36 36 val = w1_read_8(sl->master); 37 - mutex_unlock(&sl->master->mutex); 37 + mutex_unlock(&sl->master->bus_mutex); 38 38 39 39 return val; 40 40 }
+12 -12
drivers/w1/slaves/w1_ds2408.c
··· 52 52 if (!buf) 53 53 return -EINVAL; 54 54 55 - mutex_lock(&sl->master->mutex); 55 + mutex_lock(&sl->master->bus_mutex); 56 56 dev_dbg(&sl->dev, "mutex locked"); 57 57 58 58 if (w1_reset_select_slave(sl)) { 59 - mutex_unlock(&sl->master->mutex); 59 + mutex_unlock(&sl->master->bus_mutex); 60 60 return -EIO; 61 61 } 62 62 ··· 66 66 w1_write_block(sl->master, wrbuf, 3); 67 67 *buf = w1_read_8(sl->master); 68 68 69 - mutex_unlock(&sl->master->mutex); 69 + mutex_unlock(&sl->master->bus_mutex); 70 70 dev_dbg(&sl->dev, "mutex unlocked"); 71 71 return 1; 72 72 } ··· 165 165 return -EFAULT; 166 166 167 167 dev_dbg(&sl->dev, "locking mutex for write_output"); 168 - mutex_lock(&sl->master->mutex); 168 + mutex_lock(&sl->master->bus_mutex); 169 169 dev_dbg(&sl->dev, "mutex locked"); 170 170 171 171 if (w1_reset_select_slave(sl)) ··· 200 200 /* read the result of the READ_PIO_REGS command */ 201 201 if (w1_read_8(sl->master) == *buf) { 202 202 /* success! */ 203 - mutex_unlock(&sl->master->mutex); 203 + mutex_unlock(&sl->master->bus_mutex); 204 204 dev_dbg(&sl->dev, 205 205 "mutex unlocked, retries:%d", retries); 206 206 return 1; 207 207 } 208 208 } 209 209 error: 210 - mutex_unlock(&sl->master->mutex); 210 + mutex_unlock(&sl->master->bus_mutex); 211 211 dev_dbg(&sl->dev, "mutex unlocked in error, retries:%d", retries); 212 212 213 213 return -EIO; ··· 228 228 if (count != 1 || off != 0) 229 229 return -EFAULT; 230 230 231 - mutex_lock(&sl->master->mutex); 231 + mutex_lock(&sl->master->bus_mutex); 232 232 233 233 if (w1_reset_select_slave(sl)) 234 234 goto error; ··· 236 236 while (retries--) { 237 237 w1_write_8(sl->master, W1_F29_FUNC_RESET_ACTIVITY_LATCHES); 238 238 if (w1_read_8(sl->master) == W1_F29_SUCCESS_CONFIRM_BYTE) { 239 - mutex_unlock(&sl->master->mutex); 239 + mutex_unlock(&sl->master->bus_mutex); 240 240 return 1; 241 241 } 242 242 if (w1_reset_resume_command(sl->master)) ··· 244 244 } 245 245 246 246 error: 247 - mutex_unlock(&sl->master->mutex); 247 + mutex_unlock(&sl->master->bus_mutex); 248 248 return -EIO; 249 249 } 250 250 ··· 263 263 if (count != 1 || off != 0) 264 264 return -EFAULT; 265 265 266 - mutex_lock(&sl->master->mutex); 266 + mutex_lock(&sl->master->bus_mutex); 267 267 268 268 if (w1_reset_select_slave(sl)) 269 269 goto error; ··· 285 285 w1_write_block(sl->master, w1_buf, 3); 286 286 if (w1_read_8(sl->master) == *buf) { 287 287 /* success! */ 288 - mutex_unlock(&sl->master->mutex); 288 + mutex_unlock(&sl->master->bus_mutex); 289 289 return 1; 290 290 } 291 291 } 292 292 error: 293 - mutex_unlock(&sl->master->mutex); 293 + mutex_unlock(&sl->master->bus_mutex); 294 294 295 295 return -EIO; 296 296 }
+2 -2
drivers/w1/slaves/w1_ds2423.c
··· 66 66 wrbuf[0] = 0xA5; 67 67 wrbuf[1] = rom_addr & 0xFF; 68 68 wrbuf[2] = rom_addr >> 8; 69 - mutex_lock(&dev->mutex); 69 + mutex_lock(&dev->bus_mutex); 70 70 if (!w1_reset_select_slave(sl)) { 71 71 w1_write_block(dev, wrbuf, 3); 72 72 read_byte_count = 0; ··· 124 124 } else { 125 125 c -= snprintf(out_buf + PAGE_SIZE - c, c, "Connection error"); 126 126 } 127 - mutex_unlock(&dev->mutex); 127 + mutex_unlock(&dev->bus_mutex); 128 128 return PAGE_SIZE - c; 129 129 } 130 130
+4 -4
drivers/w1/slaves/w1_ds2431.c
··· 107 107 if (count == 0) 108 108 return 0; 109 109 110 - mutex_lock(&sl->master->mutex); 110 + mutex_lock(&sl->master->bus_mutex); 111 111 112 112 /* read directly from the EEPROM in chunks of W1_F2D_READ_MAXLEN */ 113 113 while (todo > 0) { ··· 126 126 off += W1_F2D_READ_MAXLEN; 127 127 } 128 128 129 - mutex_unlock(&sl->master->mutex); 129 + mutex_unlock(&sl->master->bus_mutex); 130 130 131 131 return count; 132 132 } ··· 214 214 if (count == 0) 215 215 return 0; 216 216 217 - mutex_lock(&sl->master->mutex); 217 + mutex_lock(&sl->master->bus_mutex); 218 218 219 219 /* Can only write data in blocks of the size of the scratchpad */ 220 220 addr = off; ··· 259 259 } 260 260 261 261 out_up: 262 - mutex_unlock(&sl->master->mutex); 262 + mutex_unlock(&sl->master->bus_mutex); 263 263 264 264 return count; 265 265 }
+4 -4
drivers/w1/slaves/w1_ds2433.c
··· 107 107 if ((count = w1_f23_fix_count(off, count, W1_EEPROM_SIZE)) == 0) 108 108 return 0; 109 109 110 - mutex_lock(&sl->master->mutex); 110 + mutex_lock(&sl->master->bus_mutex); 111 111 112 112 #ifdef CONFIG_W1_SLAVE_DS2433_CRC 113 113 ··· 138 138 #endif /* CONFIG_W1_SLAVE_DS2433_CRC */ 139 139 140 140 out_up: 141 - mutex_unlock(&sl->master->mutex); 141 + mutex_unlock(&sl->master->bus_mutex); 142 142 143 143 return count; 144 144 } ··· 233 233 } 234 234 #endif /* CONFIG_W1_SLAVE_DS2433_CRC */ 235 235 236 - mutex_lock(&sl->master->mutex); 236 + mutex_lock(&sl->master->bus_mutex); 237 237 238 238 /* Can only write data to one page at a time */ 239 239 idx = 0; ··· 251 251 } 252 252 253 253 out_up: 254 - mutex_unlock(&sl->master->mutex); 254 + mutex_unlock(&sl->master->bus_mutex); 255 255 256 256 return count; 257 257 }
+4 -4
drivers/w1/slaves/w1_ds2760.c
··· 31 31 if (!dev) 32 32 return 0; 33 33 34 - mutex_lock(&sl->master->mutex); 34 + mutex_lock(&sl->master->bus_mutex); 35 35 36 36 if (addr > DS2760_DATA_SIZE || addr < 0) { 37 37 count = 0; ··· 54 54 } 55 55 56 56 out: 57 - mutex_unlock(&sl->master->mutex); 57 + mutex_unlock(&sl->master->bus_mutex); 58 58 59 59 return count; 60 60 } ··· 76 76 if (!dev) 77 77 return -EINVAL; 78 78 79 - mutex_lock(&sl->master->mutex); 79 + mutex_lock(&sl->master->bus_mutex); 80 80 81 81 if (w1_reset_select_slave(sl) == 0) { 82 82 w1_write_8(sl->master, cmd); 83 83 w1_write_8(sl->master, addr); 84 84 } 85 85 86 - mutex_unlock(&sl->master->mutex); 86 + mutex_unlock(&sl->master->bus_mutex); 87 87 return 0; 88 88 } 89 89
+4 -18
drivers/w1/slaves/w1_ds2780.c
··· 60 60 if (!dev) 61 61 return -ENODEV; 62 62 63 - mutex_lock(&sl->master->mutex); 63 + mutex_lock(&sl->master->bus_mutex); 64 64 65 65 ret = w1_ds2780_do_io(dev, buf, addr, count, io); 66 66 67 - mutex_unlock(&sl->master->mutex); 67 + mutex_unlock(&sl->master->bus_mutex); 68 68 69 69 return ret; 70 70 } 71 71 EXPORT_SYMBOL(w1_ds2780_io); 72 - 73 - int w1_ds2780_io_nolock(struct device *dev, char *buf, int addr, size_t count, 74 - int io) 75 - { 76 - int ret; 77 - 78 - if (!dev) 79 - return -ENODEV; 80 - 81 - ret = w1_ds2780_do_io(dev, buf, addr, count, io); 82 - 83 - return ret; 84 - } 85 - EXPORT_SYMBOL(w1_ds2780_io_nolock); 86 72 87 73 int w1_ds2780_eeprom_cmd(struct device *dev, int addr, int cmd) 88 74 { ··· 77 91 if (!dev) 78 92 return -EINVAL; 79 93 80 - mutex_lock(&sl->master->mutex); 94 + mutex_lock(&sl->master->bus_mutex); 81 95 82 96 if (w1_reset_select_slave(sl) == 0) { 83 97 w1_write_8(sl->master, cmd); 84 98 w1_write_8(sl->master, addr); 85 99 } 86 100 87 - mutex_unlock(&sl->master->mutex); 101 + mutex_unlock(&sl->master->bus_mutex); 88 102 return 0; 89 103 } 90 104 EXPORT_SYMBOL(w1_ds2780_eeprom_cmd);
-2
drivers/w1/slaves/w1_ds2780.h
··· 124 124 125 125 extern int w1_ds2780_io(struct device *dev, char *buf, int addr, size_t count, 126 126 int io); 127 - extern int w1_ds2780_io_nolock(struct device *dev, char *buf, int addr, 128 - size_t count, int io); 129 127 extern int w1_ds2780_eeprom_cmd(struct device *dev, int addr, int cmd); 130 128 131 129 #endif /* !_W1_DS2780_H */
+4 -18
drivers/w1/slaves/w1_ds2781.c
··· 58 58 if (!dev) 59 59 return -ENODEV; 60 60 61 - mutex_lock(&sl->master->mutex); 61 + mutex_lock(&sl->master->bus_mutex); 62 62 63 63 ret = w1_ds2781_do_io(dev, buf, addr, count, io); 64 64 65 - mutex_unlock(&sl->master->mutex); 65 + mutex_unlock(&sl->master->bus_mutex); 66 66 67 67 return ret; 68 68 } 69 69 EXPORT_SYMBOL(w1_ds2781_io); 70 - 71 - int w1_ds2781_io_nolock(struct device *dev, char *buf, int addr, size_t count, 72 - int io) 73 - { 74 - int ret; 75 - 76 - if (!dev) 77 - return -ENODEV; 78 - 79 - ret = w1_ds2781_do_io(dev, buf, addr, count, io); 80 - 81 - return ret; 82 - } 83 - EXPORT_SYMBOL(w1_ds2781_io_nolock); 84 70 85 71 int w1_ds2781_eeprom_cmd(struct device *dev, int addr, int cmd) 86 72 { ··· 75 89 if (!dev) 76 90 return -EINVAL; 77 91 78 - mutex_lock(&sl->master->mutex); 92 + mutex_lock(&sl->master->bus_mutex); 79 93 80 94 if (w1_reset_select_slave(sl) == 0) { 81 95 w1_write_8(sl->master, cmd); 82 96 w1_write_8(sl->master, addr); 83 97 } 84 98 85 - mutex_unlock(&sl->master->mutex); 99 + mutex_unlock(&sl->master->bus_mutex); 86 100 return 0; 87 101 } 88 102 EXPORT_SYMBOL(w1_ds2781_eeprom_cmd);
-2
drivers/w1/slaves/w1_ds2781.h
··· 129 129 130 130 extern int w1_ds2781_io(struct device *dev, char *buf, int addr, size_t count, 131 131 int io); 132 - extern int w1_ds2781_io_nolock(struct device *dev, char *buf, int addr, 133 - size_t count, int io); 134 132 extern int w1_ds2781_eeprom_cmd(struct device *dev, int addr, int cmd); 135 133 136 134 #endif /* !_W1_DS2781_H */
+5 -5
drivers/w1/slaves/w1_therm.c
··· 179 179 int i, max_trying = 10; 180 180 ssize_t c = PAGE_SIZE; 181 181 182 - i = mutex_lock_interruptible(&dev->mutex); 182 + i = mutex_lock_interruptible(&dev->bus_mutex); 183 183 if (i != 0) 184 184 return i; 185 185 ··· 207 207 w1_write_8(dev, W1_CONVERT_TEMP); 208 208 209 209 if (external_power) { 210 - mutex_unlock(&dev->mutex); 210 + mutex_unlock(&dev->bus_mutex); 211 211 212 212 sleep_rem = msleep_interruptible(tm); 213 213 if (sleep_rem != 0) 214 214 return -EINTR; 215 215 216 - i = mutex_lock_interruptible(&dev->mutex); 216 + i = mutex_lock_interruptible(&dev->bus_mutex); 217 217 if (i != 0) 218 218 return i; 219 219 } else if (!w1_strong_pullup) { 220 220 sleep_rem = msleep_interruptible(tm); 221 221 if (sleep_rem != 0) { 222 - mutex_unlock(&dev->mutex); 222 + mutex_unlock(&dev->bus_mutex); 223 223 return -EINTR; 224 224 } 225 225 } ··· 258 258 259 259 c -= snprintf(buf + PAGE_SIZE - c, c, "t=%d\n", 260 260 w1_convert_temp(rom, sl->family->fid)); 261 - mutex_unlock(&dev->mutex); 261 + mutex_unlock(&dev->bus_mutex); 262 262 263 263 return PAGE_SIZE - c; 264 264 }
+8 -1
drivers/w1/w1.c
··· 885 885 * 886 886 * Return 0 - device(s) present, 1 - no devices present. 887 887 */ 888 + mutex_lock(&dev->bus_mutex); 888 889 if (w1_reset_bus(dev)) { 890 + mutex_unlock(&dev->bus_mutex); 889 891 dev_dbg(&dev->dev, "No devices present on the wire.\n"); 890 892 break; 891 893 } 892 894 893 895 /* Do fast search on single slave bus */ 894 896 if (dev->max_slave_count == 1) { 897 + int rv; 895 898 w1_write_8(dev, W1_READ_ROM); 899 + rv = w1_read_block(dev, (u8 *)&rn, 8); 900 + mutex_unlock(&dev->bus_mutex); 896 901 897 - if (w1_read_block(dev, (u8 *)&rn, 8) == 8 && rn) 902 + if (rv == 8 && rn) 898 903 cb(dev, rn); 899 904 900 905 break; ··· 932 927 rn |= (tmp64 << i); 933 928 934 929 if (kthread_should_stop()) { 930 + mutex_unlock(&dev->bus_mutex); 935 931 dev_dbg(&dev->dev, "Abort w1_search\n"); 936 932 return; 937 933 } 938 934 } 935 + mutex_unlock(&dev->bus_mutex); 939 936 940 937 if ( (triplet_ret & 0x03) != 0x03 ) { 941 938 if ( (desc_bit == last_zero) || (last_zero < 0))
+1
drivers/w1/w1.h
··· 180 180 181 181 struct task_struct *thread; 182 182 struct mutex mutex; 183 + struct mutex bus_mutex; 183 184 184 185 struct device_driver *driver; 185 186 struct device dev;
+1
drivers/w1/w1_int.c
··· 76 76 77 77 INIT_LIST_HEAD(&dev->slist); 78 78 mutex_init(&dev->mutex); 79 + mutex_init(&dev->bus_mutex); 79 80 80 81 memcpy(&dev->dev, device, sizeof(struct device)); 81 82 dev_set_name(&dev->dev, "w1_bus_master%u", dev->id);