W1: w1 core deadlock and oops fixes, ds2490 updates
Subsystems (2)
cover W1: w1 core deadlock and oops fixes, ds2490 updates
The Linux one wire system allows talking to little devices, mostly sensors. Sensors such as temperature sensors that I want to put around the house, but first the one wire system and ds2490 USB to one wire controller driver needs to be stable. After a kernel panic, I did most all of the development inside of qemu. I had an issue with bulk transfers failing unless I told qemu to disconnect and reconnect the USB device every time I reloaded, the module, but it worked really nicely. What follows is a long list of fixes and enhancements. I would like some tester feedback. I only have the ds2490 USB to one wire controller and ds18b20 thermometer.
This set of patches fixes bugs in the one wire driver and the ds2490 one wire USB controller. Some of the bugs will deadlock the one wire system and print out a message every second reminding you of this. Others will panic the system. One will spam with printk in an infinite loop. The w1_therm slave device driver would overflow the userspace buffer, and didn't even work properly with cat.
In the name of being nice to the rest of the system, I've eliminated a thread that was waking up and polling every second, for pretty much only startup and shutdown events. The other thread, w1_process, will now block when there isn't anything to do, and if sleeping it will be immediately woken for a new search or for preparing to exit.
ds2490 USB to one wire master driver has been improved. The original code was observed to take 3.91 seconds for a temperature conversion and reading with 0.002s user and 3.001s system times. The system was very unresponsive, mostly due to some mdelay(750) calls. Now it is taking 0.860s elapsed with 0.004s user and 0.004s system time. That is pretty good considering that the temperature conversion takes 750ms (rounded up to 752ms). Some of the 108ms overload could be reduced by a shorter reset period, overdrive data transfer speed, and combining USB operations. The driver now supports the strong pullup, which is useful for parasite powered devices.
I was keeping track of my changes in cvs. I've included the file, cvs version number and log for that commit that make up the given patch. The cvs number is just to help me keep everything organized to make the patches. The patches are against 2.6.25-rc4.
--
David Fries <[email protected]>
http://fries.net/~david/ (PGP encryption key available)
Hi David.
Quoted 10 lines · David Fries
The Linux one wire system allows talking to little devices, mostly sensors. Sensors such as temperature sensors that I want to put around the house, but first the one wire system and ds2490 USB to one wire controller driver needs to be stable. After a kernel panic, I did most all of the development inside of qemu. I had an issue with bulk transfers failing unless I told qemu to disconnect and reconnect the USB device every time I reloaded, the module, but it worked really nicely. What follows is a long list of fixes and enhancements. I would like some tester feedback. I only have the ds2490 USB to one wire controller and ds18b20 thermometer.
Well, in our private converstion you never showed kernel panic dump or at least talks about where it was located, but nevertheless I'm overall ok with your changes. I ack all ds2490 changes and generally do not object against others, although description of them some times are really wrong, like what you call a deadlock was intended - driver specially can not be unloaded while there are users of given bus.
There is another problem with your submission - please split it into 3-5 patches which are logically compound, so that there would not be things
like: patch1: replace A with B, patch2: remove B and the like. You also have lots of codying style issues in the patchset.
Quoted 6 lines
This set of patches fixes bugs in the one wire driver and the ds2490 one wire USB controller. Some of the bugs will deadlock the one wire system and print out a message every second reminding you of this. Others will panic the system. One will spam with printk in an infinite loop. The
w1_thermslave device driver would overflow the userspace buffer, and didn't even work properly withcat.
The latter is wrong, and you never showed signs of previous errors, but looking at the end results I agree that it is better solution (especially eliminating of the additional thread and related changes).
Quoted 6 lines
In the name of being nice to the rest of the system, I've eliminated a thread that was waking up and polling every second, for pretty much only startup and shutdown events. The other thread,
w1_process, will now block when there isn't anything to do, and if sleeping it will be immediately woken for a new search or for preparing to exit.
Quoted 10 lines
ds2490 USB to one wire master driver has been improved. The original code was observed to take 3.91 seconds for a temperature conversion and reading with 0.002s user and 3.001s system times. The system was very unresponsive, mostly due to some mdelay(750) calls. Now it is taking 0.860s elapsed with 0.004s user and 0.004s system time. That is pretty good considering that the temperature conversion takes 750ms (rounded up to 752ms). Some of the 108ms overload could be reduced by a shorter reset period, overdrive data transfer speed, and combining USB operations. The driver now supports the strong pullup, which is useful for parasite powered devices.
Yeah, mdelay was not a good solution.
Quoted 4 lines
I was keeping track of my changes in cvs. I've included the file, cvs version number and log for that commit that make up the given patch. The cvs number is just to help me keep everything organized to make the patches. The patches are against 2.6.25-rc4.
Please reorganize your patches into something like this:
1. threads changes
2. master device removal changes (remove slaves which are on the bus master being removed) and related things
3. ds2490 changes
4. cleanups
or something like that, reviewing 35 small patches which are removing things past another is not a really good time...
Thank you. I will comment on separate patches.
Signature · 1 line
Evgeniy Polyakov
Andrew Morton, I quoted you near the end.
Quoted 14 lines · Evgeniy Polyakov
Hi David.
Quoted 10 lines · David Fries
The Linux one wire system allows talking to little devices, mostly sensors. Sensors such as temperature sensors that I want to put around the house, but first the one wire system and ds2490 USB to one wire controller driver needs to be stable. After a kernel panic, I did most all of the development inside of qemu. I had an issue with bulk transfers failing unless I told qemu to disconnect and reconnect the USB device every time I reloaded, the module, but it worked really nicely. What follows is a long list of fixes and enhancements. I would like some tester feedback. I only have the ds2490 USB to one wire controller and ds18b20 thermometer.
Quoted 6 lines
Well, in our private converstion you never showed kernel panic dump or at least talks about where it was located, but nevertheless I'm overall ok with your changes. I ack all ds2490 changes and generally do not object against others, although description of them some times are really wrong, like what you call a deadlock was intended - driver specially can not be unloaded while there are users of given bus.
I dug out a panic before I started modifying the driver, see the end of the message. There is still a remaining oops. Register the same sensor with two masters. The serial id of the sensor is assumed to be unique and the sysfs code will cause an oops when the second sensor is registered. If you have two master devices that are both doing bus searching, let one detect the sensor, the move it to the other master. The second master will detect the device before the first one times it out. None of my patches address this.
Could you be more specific on "really wrong, like what you call a deadlock was intended"? I only use 'intended' once, and that is is patch 1 in "The slave reconnect was incorrectly coded." That paragraph is describing a infinite loop inserting a slave driver, not unloading a device driver.
If something is really wrong with the descriptions I'll fix it.
Quoted 3 lines
There is another problem with your submission - please split it into 3-5 patches which are logically compound, so that there would not be things
like: patch1: replace A with B, patch2: remove B and the like.
You must be talking about patch 14 and patch 15 w1_slave_read_id sort read bug. That's the only place it does that.
I debated on how to submit
patch 14 "w1_slave_read_id multiple short read bug"
and
patch 15 "w1_slave_read_id from bin_attribute to device_attribute" and I did it that way to first show what he bug was and fix it, then show how simple the code was using a different method. I'll submit it in one patch.
Quoted 1 line
You also have lots of codying style issues in the patchset.
Such as?
Quoted 8 lines
Quoted 6 lines
This set of patches fixes bugs in the one wire driver and the ds2490 one wire USB controller. Some of the bugs will deadlock the one wire system and print out a message every second reminding you of this. Others will panic the system. One will spam with printk in an infinite loop. The
w1_thermslave device driver would overflow the userspace buffer, and didn't even work properly withcat.The latter is wrong, and you never showed signs of previous errors,
This was the overview e-mail, and it was long enough without going into details on each of the issues. But here they are.
"will deadlock the one wire system and print out a message every second reminding you of this."
This example requires a temperature sensor that uses w1_therm (it isn't specific to w1_therm though).
insmod wire.ko
insmod slaves/w1_therm.ko
insmod masters/ds2490.ko
(wait for devices to be detected)
rmmod slaves/w1_therm
Waiting for family 40 to become free: refcnt=1.
Waiting for family 40 to become free: refcnt=1.
Waiting for family 40 to become free: refcnt=1.
...
40 = 0x28
#define W1_THERM_DS18B20 0x28
It is w1_therm that is trying to be removed, but it can't because the slave is using w1_therm. The error is nothing told the slave to stop using w1_therm, so unless you do something it never breaks out of the loop. In this case,
rmmod ds2490
will allow you to break out of it, but it is in a deadlock until then.
[PATCH 1/35] W1: fix deadlocks and remove w1_control_thread w1.c 1.2w1_family.c 1.3
Fixed the infinite loop that happens if a slave device module is removed from the kernel and there is a slave using that driver family. That involves unattaching all slaves that are using that family after the family has been removed from the list of available family drivers. This is not a race condition, it will happen everytime.
"One will spam with printk in an infinite loop."
This requires two devices that aren't used by w1_smem, the slave module doesn't matter, it just requires two devices that will not be claimed by the driver loaded.
insmod wire.ko
insmod masters/ds2490.ko
(wait for devices to be detected)
insmod slaves/w1_smem.ko
w1_slave_release: Releasing 21-34c0000039c5.w1_master_driver w1_bus_master1: Family 21 for 21.34c0000039c5.ba is not registered.w1_slave_release: Releasing 23-00000002937d.w1_master_driver w1_bus_master1: Family 23 for 23.00000002937d.84 is not registered.w1_slave_release: Releasing 28-0000000e84a2.w1_master_driver w1_bus_master1: Family 28 for 28.0000000e84a2.ed is not registered.w1_slave_release: Releasing 21-34c0000039c5.w1_master_driver w1_bus_master1: Family 21 for 21.34c0000039c5.ba is not registered.w1_slave_release: Releasing 23-00000002937d.w1_master_driver w1_bus_master1: Family 23 for 23.00000002937d.84 is not registered.w1_slave_release: Releasing 28-0000000e84a2.w1_master_driver w1_bus_master1: Family 28 for 28.0000000e84a2.ed is not registered.
21, 23, 28, LOOP
And yes, I had to hang the machine to collect this output.
[PATCH 1/35] W1: fix deadlocks and remove w1_control_thread w1.c 1.3w1.h 1.6
The slave reconnect was incorrectly coded. When a new slave device driver (and family code) was available it was intended to give slaves using the default device a chance to use the specific slave device driver. It was using the list_for_each_entry_safe and took the brute force method of any device using the default device was detached and reconnected. If there was a slave device driver that matched the family code it would be added back to the list with that driver, otherwise it would be added back with the default driver. It only takes two devices that don't match the current set of family codes to be added to the end of the list to get into an infinite loop of detach, add to the end of the list, get next entry.
(copy)
Quoted 1 line
The latter is wrong, and you never showed signs of previous errors,
Which latter are you referring to?
"The w1_therm slave device driver would overflow the userspace buffer"
Strace make it obvious, request one byte, '..., 1)', get 72, that's a buffer overflow.
cait:/sys/devices/w1_bus_master1/28-0000000e84a2$ strace -e read,open dd if=w1_slave bs=1 count=1
open("w1_slave", O_RDONLY) = 0
read(0, "00 01 4b 46 7f ff 10 10 54 : crc"..., 1) = 72
00 01 4b 46 7f ff 10 10 54 : crc=54 YES
00 01 4b 46 7f ff 10 10 54 t=16
1+0 records in
0+1 records out
72 bytes (72 B) copied, 32.8605 seconds, 0.0 kB/s
Process 2204 detached[PATCH 16/35] [W1] w1_therm fix user buffer overflow and cat slaves/w1_therm.c 1.8
buffer overflow:
Execute a short read on w1_slave and w1_therm_read_bin would still return the full string size worth of data clobbering the user space buffer when it returned. Switching to device_attribute avoids the buffer overflow problems. With the snprintf formatted output dealing with short reads without doing a conversion per read would have been difficult.
'and didn't even work properly with cat."
cait:/sys/devices/w1_bus_master1/28-0000000e84a2$ cat w1_slave 05 01 4b 46 7f ff 0b 10 cd : crc=cd YES 05 01 4b 46 7f ff 0b 10 cd t=16 05 01 4b 46 7f ff 0b 10 cd : crc=cd YES 05 01 4b 46 7f ff 0b 10 cd t=16
That's two reads, two temperature conversions when there should have only been one.
cait:/sys/devices/w1_bus_master1/28-0000000e84a2$ strace -e read,open cat w1_slave read(3, "ff 00 4b 46 7f ff 01 10 9c : crc"..., 4096) = 72 read(3, "ff 00 4b 46 7f ff 01 10 9c : crc"..., 4096) = 72 read(3, "", 4096) = 0
[PATCH 16/35] [W1] w1_therm fix user buffer overflow and cat slaves/w1_therm.c 1.8
bad behavior:
cat w1_slave would cause two temperature conversions to take place. Previously the code assumed W1_SLAVE_DATA_SIZE would be returned with each read. It would not return 0 unless the offset was less than W1_SLAVE_DATA_SIZE. The result was the first read did a temperature conversion, filled the buffer and returned, the offset in the second read would be less than W1_SLAVE_DATA_SIZE and also fill the buffer and return, the third read would finnally have a big enough offset to return 0 and cause cat to stop. Now w1_therm_read will be called at most once per open.
Interesting, now that I have more sensors connected to the network and I loaded the old in tree w1 driver (for the above tests), I see it fails to detect one sensor. The old driver consistently gets 4, while with my changes it is consistently getting 5 and there should be 5 devices out there. I must have fixed some bug without knowning it.
2.6.24 w1 system
28-0000000e84a2
81-00000020bbd0
21-34c0000039c5
23-00000002937d
submitted patch,
21-34c0000039c5
23-00000002937d
28-0000000e84a2
28-0000000e9812
81-00000020bbd0
Quoted 2 lines
but looking at the end results I agree that it is better solution (especially eliminating of the additional thread and related changes).
Thanks, it pleased me greatly to remove the w1_thread that only polled when it wasn't required.
Quoted 17 lines
Quoted 6 lines
In the name of being nice to the rest of the system, I've eliminated a thread that was waking up and polling every second, for pretty much only startup and shutdown events. The other thread,
w1_process, will now block when there isn't anything to do, and if sleeping it will be immediately woken for a new search or for preparing to exit.Quoted 10 lines
ds2490 USB to one wire master driver has been improved. The original code was observed to take 3.91 seconds for a temperature conversion and reading with 0.002s user and 3.001s system times. The system was very unresponsive, mostly due to some mdelay(750) calls. Now it is taking 0.860s elapsed with 0.004s user and 0.004s system time. That is pretty good considering that the temperature conversion takes 750ms (rounded up to 752ms). Some of the 108ms overload could be reduced by a shorter reset period, overdrive data transfer speed, and combining USB operations. The driver now supports the strong pullup, which is useful for parasite powered devices.
Quoted 1 line
Yeah, mdelay was not a good solution.
How's the status of your one wire network? What kind of sensors do you have available?
Quoted 12 lines
Quoted 4 lines
I was keeping track of my changes in cvs. I've included the file, cvs version number and log for that commit that make up the given patch. The cvs number is just to help me keep everything organized to make the patches. The patches are against 2.6.25-rc4.
Please reorganize your patches into something like this:
1. threads changes
2. master device removal changes (remove slaves which are on the bus master being removed) and related things
3. ds2490 changes
4. cleanups
Quoted 2 lines
or something like that, reviewing 35 small patches which are removing things past another is not a really good time...
You want me to take 35 patches and merge them down to 4? There were a lot of issues in here, and I don't have an automagic patch reorder tool. If you want to review fewer patches apply 1-6, do a diff and look at that.
Documentation/SubmittingPatches '3) Separate your changes.' It goes on to specifically separate out bug fixes and performance enhancements.
You requested in another e-mail to merge patch 1-6. The deadlocks in 1 is clearly a bug. 6 block (instead of sleeping with a timeout) is a performance enhancement. 5 might be a deadlock, but it's not at all related to 1. 2 and 3 have nothing to do with any of the above. Logically this come out to be a bunch of patches.
Quoted 5 lines · Andrew Morton
Quoted 2 lines · On Mon, 21 Jan 2008 20:47:19 -0600 David Fries <[email protected]> wrote:
Thanks. Question, at what point should I just submit these one liner, but independent bugs in one patch?
You shouldn't, unless they are dependent in both directions.
If Andrew Morton is going to say no for independent one line bugs, I'm going to say No (for a bunch of complicated sometimes overlapping bugs, features, and performance enhancements) as well. Besides I have something like 27KB in 664 lines of descriptions, if I tried to merge the changes down there's no way I could figure out which patch lines went with what descriptions, and no one else could either.
Quoted 4 lines
Thank you. I will comment on separate patches.
Signature · 1 line
Evgeniy Polyakov
Signature · 150 lines
David Fries <[email protected]> http://fries.net/~david/ (PGP encryption key available) ohci_hcd: 2006 August 04 USB 1.1 'Open' Host Controller (OHCI) Driver ACPI: PCI Interrupt 0000:00:02.2[D] -> Link [LNKD] -> GSI 11 (level, low) -> IRQ 11 ohci_hcd 0000:00:02.2: OHCI Host Controller ohci_hcd 0000:00:02.2: new USB bus registered, assigned bus number 1 ohci_hcd 0000:00:02.2: irq 11, io mem 0xdfffe000 usb usb1: configuration #1 chosen from 1 choice hub 1-0:1.0: USB hub found hub 1-0:1.0: 3 ports detected ACPI: PCI Interrupt 0000:00:02.3[D] -> Link [LNKD] -> GSI 11 (level, low) -> IRQ 11 ohci_hcd 0000:00:02.3: OHCI Host Controller ohci_hcd 0000:00:02.3: new USB bus registered, assigned bus number 2 ohci_hcd 0000:00:02.3: irq 11, io mem 0xdffff000 usb usb2: configuration #1 chosen from 1 choice usb 1-2: new low speed USB device using ohci_hcd and address 2 hub 2-0:1.0: USB hub found hub 2-0:1.0: 3 ports detected usb 1-2: configuration #1 chosen from 1 choice hiddev96: USB HID v1.00 Device [American Power Conversion BackUPS Pro 500 FW:16.3.D USB FW:4] on usb-0000:00:02.2-2 usb 2-1: new full speed USB device using ohci_hcd and address 2 usb 2-1: configuration #1 chosen from 1 choice drivers/usb/class/usblp.c: usblp0: USB Bidirectional printer dev 2 if 0 alt 0 proto 2 vid 0x04A9 pid 0x1056 usb 2-1: USB disconnect, address 2 drivers/usb/class/usblp.c: usblp0: removed usb 2-1: new full speed USB device using ohci_hcd and address 3 usb 2-1: configuration #1 chosen from 1 choice drivers/usb/class/usblp.c: usblp0: USB Bidirectional printer dev 3 if 0 alt 0 proto 2 vid 0x04A9 pid 0x1056 usb 2-1: USB disconnect, address 3 drivers/usb/class/usblp.c: usblp0: removed usb 2-1: new full speed USB device using ohci_hcd and address 4 usb 2-1: configuration #1 chosen from 1 choice drivers/usb/class/usblp.c: usblp0: USB Bidirectional printer dev 4 if 0 alt 0 proto 2 vid 0x04A9 pid 0x1056 usb 2-3: new full speed USB device using ohci_hcd and address 5 usb 2-3: configuration #1 chosen from 1 choice Driver for 1-wire Dallas network protocol. usbcore: registered new interface driver DS9490R w1_master_driver w1_bus_master1: Family 81 for 81.00000020bbd0.21 is not registered. w1_slave_release: Releasing 81-00000020bbd0. w1_master_driver w1_bus_master1: Family 81 for 81.00000020bbd0.21 is not registered. w1_slave_release: Releasing 81-00000020bbd0. 0x81: count=16, status: 00 00 2f 40 05 04 04 00 20 29 00 00 00 01 00 00 enable flag: 0 1-wire speed: 0 strong pullup duration: 2f programming pulse duration: 40 pulldown slew rate control: 5 write-1 low time: 4 data sample offset/write-0 recovery time: 4 reserved (test register): 0 device status flags: 20 communication command byte 1: 29 communication command byte 2: 0 communication command buffer status: 0 1-wire data output buffer status: 0 1-wire data input buffer status: 1 reserved: 0 reserved: 0 Clearing ep0x83. 0x81: count=16, status: 00 00 2f 40 05 04 04 00 20 29 00 00 00 00 00 00 enable flag: 0 1-wire speed: 0 strong pullup duration: 2f programming pulse duration: 40 pulldown slew rate control: 5 write-1 low time: 4 data sample offset/write-0 recovery time: 4 reserved (test register): 0 device status flags: 20 communication command byte 1: 29 communication command byte 2: 0 communication command buffer status: 0 1-wire data output buffer status: 0 1-wire data input buffer status: 0 reserved: 0 reserved: 0 w1_master_driver w1_bus_master1: Family 21 for 21.34c0000039c5.ba is not registered. Failed to read 1-wire data from 0x02: err=-110. Failed to read 1-wire data from 0x02: err=-110. usb 2-3: USB disconnect, address 5 w1_master_driver w1_bus_master1: Waiting for w1_bus_master1 to become free: refcnt=1. w1_slave_release: Releasing 81-00000020bbd0. w1_slave_release: Releasing 21-34c0000039c5. usb 2-3: new full speed USB device using ohci_hcd and address 6 usb 2-3: configuration #1 chosen from 1 choice usb 2-3: USB disconnect, address 6 usb 2-3: new full speed USB device using ohci_hcd and address 7 usb 2-3: configuration #1 chosen from 1 choice usb 2-3: USB disconnect, address 7 w1_slave_release: Releasing 28-0000000e84a2. w1_slave_release: Releasing 81-00000020bbd0. usb 2-3: new full speed USB device using ohci_hcd and address 8 usb 2-3: device descriptor read/64, error -62 hub 2-0:1.0: Cannot enable port 3. Maybe the USB cable is bad? hub 2-0:1.0: Cannot enable port 3. Maybe the USB cable is bad? hub 2-0:1.0: Cannot enable port 3. Maybe the USB cable is bad? hub 2-0:1.0: Cannot enable port 3. Maybe the USB cable is bad? usb 2-3: new full speed USB device using ohci_hcd and address 12 usb 2-3: configuration #1 chosen from 1 choice usb 2-3: USB disconnect, address 12 BUG: unable to handle kernel NULL pointer dereference at virtual address 0000000c printing eip: c02fa9a6 *pde = 00000000 Oops: 0000 [#1] PREEMPT Modules linked in: w1_smem w1_therm ds2490 wire cn ohci_hcd it87 hwmon_vid hwmon i2c_isa eeprom i2c_sis96x cx8800 cx88_dvb cx8802 cx88xx videodev tuner v4l1_compat v4l2_common nf_conntrack_ipv4 nfsd exportfs xt_state nf_conntrack nfnetlink ipt_REJECT iptable_filter ip_tables parport_pc parport floppy snd_intel8x0 snd_intel8x0m snd_ac97_codec ac97_bus snd_pcm_oss snd_mixer_oss snd_pcm snd_timer snd soundcore snd_page_alloc xt_tcpudp xt_multiport x_tables compat_ioctl32 cx88_vp3054_i2c mt352 ir_common i2c_algo_bit tveeprom btcx_risc dvb_pll or51132 video_buf_dvb video_buf nxt200x firmware_class isl6421 zl10353 cx24123 lgdt330x dvb_core cx22702 i2c_core nfs lockd sunrpc udf isofs nls_base usblp usbhid usbcore ff_memless sis900 CPU: 0 EIP: 0060:[<c02fa9a6>] Not tainted VLI EFLAGS: 00010282 (2.6.22 #10) EIP is at klist_del+0x6/0x50 eax: 00000000 ebx: da0012a0 ecx: dba80000 edx: d8f05780 esi: da0012a0 edi: dba80ebc ebp: 00000000 esp: dba80e4c ds: 007b es: 007b fs: 0000 gs: 0000 ss: 0068 Process khubd (pid: 1079, ti=dba80000 task=c148ea50 task.ti=dba80000) Stack: da0012a0 e0bb6100 c02faaa8 da001274 c0218dd1 da001274 da001200 c02191cd da001274 c02187eb da001274 c02171aa da001274 da001200 dba80ebc 00000000 c0217228 ca46fcc0 e0bb323d c0110af8 00000000 00000000 df25e250 c02fa970 Call Trace: [<c02faaa8>] klist_remove+0x8/0x20 [<c0218dd1>] __device_release_driver+0x31/0x90 [<c02191cd>] device_release_driver+0x1d/0x40 [<c02187eb>] bus_remove_device+0x5b/0x70 [<c02171aa>] device_del+0x19a/0x210 [<c0217228>] device_unregister+0x8/0x10 [<e0bb323d>] __w1_remove_master_device+0x8d/0xa0 [wire] [<c0110af8>] complete+0x48/0x70 [<c02fa970>] klist_release+0x0/0x30 [<e0b7b051>] ds_disconnect+0x41/0x60 [ds2490] [<e0a4470a>] usb_unbind_interface+0x2a/0x60 [usbcore] [<c0218e0e>] __device_release_driver+0x6e/0x90 [<c02191cd>] device_release_driver+0x1d/0x40 [<c02187eb>] bus_remove_device+0x5b/0x70 [<c02171aa>] device_del+0x19a/0x210 [<e0a4238c>] usb_disable_device+0x5c/0xc0 [usbcore] [<e0a3ebe2>] usb_disconnect+0x82/0x100 [usbcore] [<e0a3f15d>] hub_thread+0x31d/0xa10 [usbcore] [<c010f82c>] __activate_task+0x1c/0x30 [<c02fb59f>] schedule+0x48f/0x550 [<c02fb638>] schedule+0x528/0x550 [<c0123990>] autoremove_wake_function+0x0/0x40 [<e0a3ee40>] hub_thread+0x0/0xa10 [usbcore] [<c01238b8>] kthread+0x38/0x60 [<c0123880>] kthread+0x0/0x60 [<c0104187>] kernel_thread_helper+0x7/0x10 ======================= Code: 04 89 42 04 89 10 c7 43 f8 00 01 10 00 c7 41 04 00 02 20 00 8d 43 04 e8 19 61 e1 ff c7 43 f4 00 00 00 00 5b c3 56 53 89 c6 8b 00 <8b> 58 0c b8 01 00 00 00 e8 5d 51 e1 ff 89 f0 e8 a6 ff ff ff 85 EIP: [<c02fa9a6>] klist_del+0x6/0x50 SS:ESP 0068:dba80e4c
PATCH 7/35 W1: feature, enable hardware strong pullup
===================================================================
This is an update to the previous set of patches, don't merge them yet. Some of the context was messed up and I don't expect patch to apply the updates with the previous set cleanly. I'll resubmit the entire set if these updates look good.
===================================================================
Add a strong pullup option to the w1 system. This supplies extra power for parasite powered devices.
The one wire bus requires at a minimum one wire and ground. The common wire is used for sending and receiving data as well as supplying power to devices that are parasite powered. Temperature sensors are one device that can be parasite powered and require no bus transactions while the temperature conversion is progress. A bus transaction would pull the common wire down. In addition to the normal pullup resistor, which allows a device to pull the bus line down to send data, some devices support a strong pullup mechanism to deliver more power. The documentation lists this as useful for larger networks and when temperature sensors are exposed to higher temperatures they can exceed the current supplied by a simple pullup resistor. This patch adds a mechanism to support a strong pullup, and fall back to sleeping with the line high otherwise. This patch doesn't enable the strong pullup for any master or slave device, those will follow as they require this patch.
The hardware USB 2490 one wire bus master has a bit on a few commands which will enable the strong pullup as soon as the command finishes executing. That requires the ds2490 driver to be notified if it should be set before the command is executed. That's why I added the w1_next_pullup. That will call the master set_pullup just before the next write, and reset the value to zero after it has completed. If the master driver doesn't support the set_pullup, it will delay the appropriate amount of time after the write returns.
w1.h 1.2 1.3 1.4 1.10
Add set_pullup to w1_bus_master, and add pullup_duration to w1_master to hold the strong pullup duration for the next write operation. Add enable_pullup to w1_master for a per master enable/disable option.
w1.c 1.20
Add w1_master_pullup sysfs entry.
w1_int.c 1.3 1.9
Add a check to disable set_pullup and print a warning message when the operation isn't supported. Add enable_pullup module parameter.
w1_io.c 1.2 1.3 1.4 1.6
Add w1_next_pullup, call w1_pre_write and w1_post_write to call set_pullup before and after, or just sleep after if set_pullup isn't supported.
--- drivers/w1/w1.c | 30 ++++++++++++++++++++++ drivers/w1/w1.h | 12 +++++++++ drivers/w1/w1_int.c | 16 ++++++++++++ drivers/w1/w1_io.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 122 insertions(+), 4 deletions(-)
drivers/w1/w1.c +30 -0
@@ -270,6 +270,34 @@ static ssize_t w1_master_attribute_show_search(struct device *dev,return count; } +static ssize_t w1_master_attribute_store_pullup (struct device * dev, +struct device_attribute *attr, +const char * buf,size_t count) +{ +struct w1_master *md =dev_to_w1_master (dev); + +mutex_lock (&md->mutex); + md->enable_pullup =simple_strtol (buf,NULL ,0 ); +mutex_unlock (&md->mutex); +wake_up_process (md->thread); + +return count; +} + +static ssize_t w1_master_attribute_show_pullup (struct device *dev, +struct device_attribute *attr, +char *buf) +{ +struct w1_master *md =dev_to_w1_master (dev); +ssize_t count; + +mutex_lock (&md->mutex); + count =sprintf (buf,"%d\n" , md->enable_pullup); +mutex_unlock (&md->mutex); + +return count; +} +static ssize_t w1_master_attribute_show_pointer (struct device *dev,struct device_attribute *attr,char *buf) {struct w1_master *md =dev_to_w1_master (dev);
@@ -365,6 +393,7 @@ static W1_MASTER_ATTR_RO(attempts, S_IRUGO); static W1_MASTER_ATTR_RO(timeout, S_IRUGO); static W1_MASTER_ATTR_RO(pointer, S_IRUGO); static W1_MASTER_ATTR_RW(search, S_IRUGO | S_IWUGO); +static W1_MASTER_ATTR_RW(pullup, S_IRUGO | S_IWUGO);static struct attribute *w1_master_default_attrs[] = { &w1_master_attribute_name.attr,
@@ -375,6 +404,7 @@ static struct attribute *w1_master_default_attrs[] = { &w1_master_attribute_timeout.attr, &w1_master_attribute_pointer.attr, &w1_master_attribute_search.attr, + &w1_master_attribute_pullup.attr, NULL };
drivers/w1/w1.h +12 -0
@@ -142,6 +142,12 @@ struct w1_bus_master*/ u8 (*reset_bus)(void *); +/** +* Put out a strong pull-up pulse of the specified duration. +* @return -1=Error, 0=completed +*/ +u8 (*set_pullup)(void *,int ); +/** Really nice hardware can handles the different types of ROM search * w1_master* is passed to the slave found callback. */
@@ -167,6 +173,11 @@ struct w1_mastervoid *priv;int priv_size; +/** 5V strong pullup enabled flag, 1 enabled, zero disabled. */ +int enable_pullup; +/** 5V strong pullup duration in milliseconds, zero disabled. */ +int pullup_duration; +struct task_struct *thread;struct mutex mutex;
@@ -201,6 +212,7 @@ u8 w1_calc_crc8(u8 *, int);void w1_write_block (struct w1_master *,const u8 *,int );u8 w1_read_block (struct w1_master *,u8 *,int );int w1_reset_select_slave (struct w1_slave *sl); +void w1_next_pullup (struct w1_master *,int );static inline struct w1_slave *dev_to_w1_slave (struct device *dev) {
drivers/w1/w1_int.c +16 -0
@@ -31,6 +31,9 @@static u32 w1_ids =1 ; +static int w1_enable_pullup =1 ; +module_param_named (enable_pullup, w1_enable_pullup, int,0 ); +static struct w1_master *w1_alloc_dev (u32 id,int slave_count,int slave_ttl,struct device_driver *driver,struct device *device)
@@ -59,6 +62,7 @@ static struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl, dev->initialized =0 ; dev->id = id; dev->slave_ttl = slave_ttl; + dev->enable_pullup = w1_enable_pullup; dev->search_count =-1 ;/* continual scan */ /* 1 for w1_process to decrement
@@ -107,6 +111,18 @@ int w1_add_master_device(struct w1_bus_master *master)printk (KERN_ERR "w1_add_master_device: invalid function set\n" );return (-EINVAL ); } +/* While it would be electrically possible to make a device that +* generated a strong pullup in bit bang mode, only hardare that +* controls 1-wire time frames are even expected to support a strong +* pullup. w1_io.c would need to support calling set_pullup before +* the last write_bit operation of a w1_write_8 which it currently +* doesn't. +*/ +if (!master->write_byte && !master->touch_bit && master->set_pullup) { +printk (KERN_ERR "w1_add_master_device: set_pullup requires " +"write_byte or touch_bit, disabling\n" ); + master->set_pullup=NULL ; + } dev =w1_alloc_dev (w1_ids++, w1_max_slave_count, w1_max_slave_ttl, &w1_master_driver, &w1_master_device);if (!dev)
drivers/w1/w1_io.c +64 -4
95 lines not on this page · raw message
Signature · 1 line
1.4.4.4
PATCH 9/35 W1: feature, w1_therm.c use strong pullup and documentation
In reply to David Fries===================================================================
This is an update to the previous set of patches, don't merge them yet. Some of the context was messed up and I don't expect patch to apply the updates with the previous set cleanly. I'll resubmit the entire set if these updates look good.
===================================================================
w1_therm.c 1.3 1.4 1.10 1.11
Use the strong pullup not the sleep. Note that the pullup goes before W1_CONVERT_TEMP not after.
+w1_next_pullup(dev, tm);
w1_write_8(dev, W1_CONVERT_TEMP);
-msleep(tm);
Also added a module parameter strong_pullup to disable the strong pullup and sleep if requested.
New directory for temperature sensor documentation.
Documentation/w1/00-INDEX 1.2
Added new subdirectory slaves
Documentation/w1/slaves/00-INDEX 1.1 1.2
Added index and w1_therm entry.
Documentation/w1/slaves/w1_therm 1.1 1.2 1.3
Initial entry for the temperature sensor.
--- Documentation/w1/00-INDEX | 2 + Documentation/w1/slaves/00-INDEX | 4 +++ Documentation/w1/slaves/w1_therm | 41 ++++++++++++++++++++++++++++++++++++++ drivers/w1/slaves/w1_therm.c | 15 ++++++++++++- 4 files changed, 60 insertions(+), 2 deletions(-)
Documentation/w1/00-INDEX +2 -0
@@ -1,5 +1,7 @@ 00-INDEX - This file +slaves/ + - Drivers that provide support for specific family codes. masters/ - Individual chips providing 1-wire busses. w1.generic
Documentation/w1/slaves/00-INDEX new file +4 -0
@@ -0,0 +1,4 @@ +00-INDEX + - This file +w1_therm + - The Maxim/Dallas Semiconductor ds18*20 temperature sensor.
Documentation/w1/slaves/w1_therm new file +41 -0
41 lines not on this page · raw message
drivers/w1/slaves/w1_therm.c +13 -2
@@ -37,6 +37,14 @@ MODULE_LICENSE("GPL");MODULE_AUTHOR ("Evgeniy Polyakov <[email protected]>" );MODULE_DESCRIPTION ("Driver for 1-wire Dallas network protocol, temperature family." ); +/* Allow the strong pullup to be disabled, but default to enabled. +* If it was disabled a parasite powered device might not get the require +* current to do a temperature conversion. If it is enabled parasite powered +* devices have a better chance of getting the current required. +*/ +static int w1_strong_pullup=1 ; +module_param_named (strong_pullup, w1_strong_pullup, int,0 ); +static u8 bad_roms[][9 ] = { {0xaa ,0x00 ,0x4b ,0x46 ,0xff ,0xff ,0x0c ,0x10 ,0x87 }, {}
@@ -192,9 +200,12 @@ static ssize_t w1_therm_read_bin(struct kobject *kobj,int count =0 ;unsigned int tm =750 ; +/* 750ms strong pullup (or delay) after the convert */ +if (w1_strong_pullup) +w1_next_pullup (dev, tm);w1_write_8 (dev,W1_CONVERT_TEMP ); - -msleep (tm); +if (!w1_strong_pullup) +msleep (tm);if (!w1_reset_select_slave (sl)) {
Signature · 1 line
1.4.4.4
PATCH 10/35 W1: be able to manually add and remove slaves
In reply to David Fries===================================================================
This is an update to the previous set of patches, don't merge them yet. Some of the context was messed up and I don't expect patch to apply the updates with the previous set cleanly. I'll resubmit the entire set if these updates look good.
===================================================================
w1.c 1.11 1.12 1.14
I'm intending to be working with a static network and run without the bus searching, but bus searching is currently the only way to add slave devices. This allows any slave to be added or removed. There is one file for adding and another for removing. If the files are read a short comment including the expected serial number format is returned.
--- drivers/w1/w1.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 137 insertions(+), 3 deletions(-)
drivers/w1/w1.c +137 -3
@@ -56,6 +56,9 @@ module_param_named(slave_ttl, w1_max_slave_ttl, int, 0);DEFINE_MUTEX (w1_mlock);LIST_HEAD (w1_masters); +static int w1_attach_slave_device (struct w1_master *dev,struct w1_reg_num *rn); +void w1_slave_detach (struct w1_slave *sl); +static int w1_master_match (struct device *dev,struct device_driver *drv) {return 1 ;
@@ -349,7 +352,8 @@ static ssize_t w1_master_attribute_show_slave_count(struct device *dev, struct dreturn count; } -static ssize_t w1_master_attribute_show_slaves (struct device *dev,struct device_attribute *attr,char *buf) +static ssize_t w1_master_attribute_show_slaves (struct device *dev, +struct device_attribute *attr,char *buf) {struct w1_master *md =dev_to_w1_master (dev);int c =PAGE_SIZE ;
@@ -374,6 +378,134 @@ static ssize_t w1_master_attribute_show_slaves(struct device *dev, struct devicereturn PAGE_SIZE - c; } +static ssize_t w1_master_attribute_show_add (struct device *dev, +struct device_attribute *attr,char *buf) +{ +int c =PAGE_SIZE ; + c -=snprintf (buf+PAGE_SIZE - c, c, +"write device id xx-xxxxxxxxxxxx to add slave\n" ); +return PAGE_SIZE - c; +} + +static int w1_atoreg_num (struct device * dev,const char *buf,size_t count, +struct w1_reg_num *rn) +{ +unsigned int family; +u64 id; +int i; +u64 rn64_le; +/* The CRC value isn't read from the user because the sysfs directory +* doesn't include it and most messages from the bus search don't +* print it either. It would be unreasonable for the user to then +* provide it. +*/ +const char *error_msg="bad slave string format, expecting " +"ff-dddddddddddd\n" ; + +if (buf[2 ]!='-' ) { +dev_err (dev,"%s" , error_msg); +return -EINVAL ; + } + i=sscanf (buf,"%02x-%012llx" , &family, &id); +if (i!=2 ) { +dev_err (dev,"%s" , error_msg); +return -EINVAL ; + } + rn->family=family; + rn->id=id; + + rn64_le=cpu_to_le64 (*(u64 *)rn); + rn->crc=w1_calc_crc8 ((u8 *)&rn64_le,7 ); + +#if 0 +dev_info (dev,"With CRC device is %02x.%012llx.%02x.\n" , + rn->family, (unsigned long long )rn->id, rn->crc); +#endif + +return 0 ; +} + +/* Searches the slaves in the w1_master and returns a pointer or NULL. +* Note: must hold the mutex +*/ +static struct w1_slave *w1_slave_search_device (struct w1_master *dev, +struct w1_reg_num *rn) +{ +struct w1_slave *sl; +list_for_each_entry (sl, &dev->slist, w1_slave_entry) { +if (sl->reg_num.family == rn->family && + sl->reg_num.id == rn->id && + sl->reg_num.crc == rn->crc) { +return sl; + } + } +return NULL ; +} + +static ssize_t w1_master_attribute_store_add (struct device * dev, +struct device_attribute *attr, +const char * buf,size_t count) +{ +struct w1_master *md =dev_to_w1_master (dev); +struct w1_reg_num rn; +struct w1_slave *sl; +ssize_t result=count; + +if (w1_atoreg_num (dev, buf, count, &rn)) +return -EINVAL ; + +mutex_lock (&md->mutex); + sl=w1_slave_search_device (md, &rn); +/* It would be nice to do a targeted search one the one-wire bus +* for the new device to see if it is out there or not. But the +* current search doesn't support that. +*/ +if (sl) { +dev_info (dev,"Device %s already exists\n" , sl->name); + result=-EINVAL ; + }else { +w1_attach_slave_device (md, &rn); + } +mutex_unlock (&md->mutex); + +return result; +} + +static ssize_t w1_master_attribute_show_remove (struct device *dev, +struct device_attribute *attr,char *buf) +{ +int c =PAGE_SIZE ; + c -=snprintf (buf+PAGE_SIZE - c, c, +"write device id xx-xxxxxxxxxxxx to remove slave\n" ); +return PAGE_SIZE - c; +} + +static ssize_t w1_master_attribute_store_remove (struct device * dev, +struct device_attribute *attr, +const char * buf,size_t count) +{ +struct w1_master *md =dev_to_w1_master (dev); +struct w1_reg_num rn; +struct w1_slave *sl; +ssize_t result=count; + +if (w1_atoreg_num (dev, buf, count, &rn)) +return -EINVAL ; + +mutex_lock (&md->mutex); + sl=w1_slave_search_device (md, &rn); +if (sl) { +w1_slave_detach (sl); + }else { +dev_info (dev,"Device %02x-%012llx doesn't exists\n" , rn.family, + (unsigned long long )rn.id); + result=-EINVAL ; + } +mutex_unlock (&md->mutex); + +return result; +} +#define W1_MASTER_ATTR_RO (_name, _mode) \ struct device_attribute w1_master_attribute_##_name = \ __ATTR(w1_master_##_name, _mode, \
@@ -394,6 +526,8 @@ static W1_MASTER_ATTR_RO(timeout, S_IRUGO); static W1_MASTER_ATTR_RO(pointer, S_IRUGO); static W1_MASTER_ATTR_RW(search, S_IRUGO | S_IWUGO); static W1_MASTER_ATTR_RW(pullup, S_IRUGO | S_IWUGO); +static W1_MASTER_ATTR_RW(add, S_IRUGO | S_IWUGO); +static W1_MASTER_ATTR_RW(remove, S_IRUGO | S_IWUGO);static struct attribute *w1_master_default_attrs[] = { &w1_master_attribute_name.attr,
@@ -405,6 +539,8 @@ static struct attribute *w1_master_default_attrs[] = { &w1_master_attribute_pointer.attr, &w1_master_attribute_search.attr, &w1_master_attribute_pullup.attr, + &w1_master_attribute_add.attr, + &w1_master_attribute_remove.attr, NULL };
@@ -700,9 +836,7 @@ void w1_reconnect_slaves(struct w1_family *f, int attach)static void w1_slave_found (struct w1_master *dev,u64 rn) { -int slave_count;struct w1_slave *sl; -struct list_head *ent;struct w1_reg_num *tmp;u64 rn_le =cpu_to_le64 (rn);
Signature · 1 line
1.4.4.4
PATCH 12/35 W1: new module parameter search_count
In reply to David Fries===================================================================
This is an update to the previous set of patches, don't merge them yet. Some of the context was messed up and I don't expect patch to apply the updates with the previous set cleanly. I'll resubmit the entire set if these updates look good.
===================================================================
w1_int.c 1.6 1.10
Added a new module parameter search_count which allows overriding the default search count. -1 continual, 0 disabled, N that many times.
--- drivers/w1/w1_int.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
drivers/w1/w1_int.c +3 -1
@@ -30,6 +30,8 @@#include "w1_int.h" static u32 w1_ids =1 ; +static int w1_search_count =-1 ;/* Default is continual scan */ +module_param_named (search_count, w1_search_count, int,0 );static int w1_enable_pullup =1 ;module_param_named (enable_pullup, w1_enable_pullup, int,0 );
@@ -62,8 +64,8 @@ static struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl, dev->initialized =0 ; dev->id = id; dev->slave_ttl = slave_ttl; + dev->search_count = w1_search_count; dev->enable_pullup = w1_enable_pullup; - dev->search_count =-1 ;/* continual scan */ /* 1 for w1_process to decrement * 1 for __w1_remove_master_device to decrement
Signature · 1 line
1.4.4.4
PATCH 13/35 W1: Document add, remove, search_count, and pullup.
In reply to David Fries===================================================================
This is an update to the previous set of patches, don't merge them yet. Some of the context was messed up and I don't expect patch to apply the updates with the previous set cleanly. I'll resubmit the entire set if these updates look good.
===================================================================
Documentation/w1/w1.generic 1.2 1.3
Document w1_master_add, w1_master_remove, search_count, and pullup.
--- Documentation/w1/w1.generic | 11 ++++++++++- 1 files changed, 10 insertions(+), 1 deletions(-)
Documentation/w1/w1.generic +10 -1
@@ -79,10 +79,13 @@ w1 master sysfs interface <xx-xxxxxxxxxxxxx> - a directory for a found device. The format is family-serial bus - (standard) symlink to the w1 bus driver - (standard) symlink to the w1 driver +w1_master_add - Manually register a slave device w1_master_attempts - the number of times a search was attempted w1_master_max_slave_count - the maximum slaves that may be attached to a master w1_master_name - the name of the device (w1_bus_masterX) +w1_master_pullup - 5V strong pullup 0 enabled, 1 disabled +w1_master_remove - Manually remove a slave device w1_master_search - the number of searches left to do, -1=continual (default) w1_master_slave_count - the number of slaves found
@@ -90,7 +93,13 @@ w1_master_slaves - the names of the slaves, one per line w1_master_timeout - the delay in seconds between searches If you have a w1 bus that never changes (you don't add or remove devices), -you can set w1_master_search to a positive value to disable searches. +you can set the module parameter search_count to a small positive number +for an initially small number of bus searches. Alternatively it could be +set to zero, then manually add the slave device serial numbers by +w1_master_add device file. The w1_master_add and w1_master_remove files +generally only make sense when searching is disabled, as a search will +redetect manually removed devices that are present and timeout manually +added devices that aren't on the bus. w1 slave sysfs interface
Signature · 1 line
1.4.4.4
PATCH 15/35 W1: w1_slave_read_id multiple short read bug
In reply to David Fries===================================================================
This is an update to the previous set of patches, don't merge them yet. Some of the context was messed up and I don't expect patch to apply the updates with the previous set cleanly. I'll resubmit the entire set if these updates look good.
===================================================================
This is more to point out the bug in w1_slave_read_id, the next patch rewrites the routine.
w1.c 1.15
Reading at an offset other than zero, ie reading less than 8 bytes at a time would result in reading the first bytes over and over until 8 bytes were returned. Added the offset to the buffer.
- memcpy(buf, (u8 *)&sl->reg_num, count);
+ memcpy(buf, (u8 *)&sl->reg_num+off, count);
But there is a better way...
w1.c 1.16
Switching w1_slave_read_id from being a bin_attribute to a device_attribute. It only has to return 8 bytes per read and lets kobject handle the buffering. That simplifies the logic in w1_slave_read_id.
--- drivers/w1/w1.c | 35 ++++++++++------------------------- 1 files changed, 10 insertions(+), 25 deletions(-)
drivers/w1/w1.c +10 -25
@@ -104,35 +104,20 @@ static ssize_t w1_slave_read_name(struct device *dev, struct device_attribute *areturn sprintf (buf,"%s\n" , sl->name); } -static ssize_t w1_slave_read_id (struct kobject *kobj, -struct bin_attribute *bin_attr, -char *buf,loff_t off,size_t count) +static ssize_t w1_slave_read_id (struct device *dev, +struct device_attribute *attr,char *buf) { -struct w1_slave *sl =kobj_to_w1_slave (kobj); - -if (off >8 ) { - count =0 ; - }else { -if (off + count >8 ) - count =8 - off; - -memcpy (buf, (u8 *)&sl->reg_num, count); - } +struct w1_slave *sl =dev_to_w1_slave (dev); +ssize_t count=sizeof (sl->reg_num); +memcpy (buf, (u8 *)&sl->reg_num, count);return count; }static struct device_attribute w1_slave_attr_name =__ATTR (name,S_IRUGO , w1_slave_read_name,NULL ); - -static struct bin_attribute w1_slave_attr_bin_id = { - .attr = { - .name ="id" , - .mode =S_IRUGO , - }, - .size =8 , - .read = w1_slave_read_id, -}; +static struct device_attribute w1_slave_attr_id = +__ATTR (id,S_IRUGO , w1_slave_read_id,NULL );/* Default family */
@@ -642,7 +627,7 @@ static int __w1_attach_slave_device(struct w1_slave *sl) }/* Create "id" entry */ - err =sysfs_create_bin_file (&sl->dev.kobj, &w1_slave_attr_bin_id); + err =device_create_file (&sl->dev, &w1_slave_attr_id);if (err <0 ) { dev_err(&sl->dev, "sysfs file creation for [%s] failed. err=%d\n",
@@ -664,7 +649,7 @@ static int __w1_attach_slave_device(struct w1_slave *sl)return 0 ; out_rem2: -sysfs_remove_bin_file (&sl->dev.kobj, &w1_slave_attr_bin_id); +device_remove_file (&sl->dev, &w1_slave_attr_id); out_rem1:device_remove_file (&sl->dev, &w1_slave_attr_name); out_unreg:
@@ -746,7 +731,7 @@ void w1_slave_detach(struct w1_slave *sl) msg.type =W1_SLAVE_REMOVE ;w1_netlink_send (sl->master, &msg); -sysfs_remove_bin_file (&sl->dev.kobj, &w1_slave_attr_bin_id); +device_remove_file (&sl->dev, &w1_slave_attr_id);device_remove_file (&sl->dev, &w1_slave_attr_name);device_unregister (&sl->dev);
Signature · 1 line
1.4.4.4
Re: [PATCH 13/33 replaces 14/35 and 15/35] W1: w1_slave_read_id multiple short read bug
Hi David.
Quoted 4 lines · David Fries
This is more to point out the bug in
w1_slave_read_id, the next patch rewrites the routine.
w1.c1.15
...
Quoted 1 line
w1.c1.16
That is what I talked about. I agree with your analysis of the problem and I agree with fixes. But I do not agree with how you created them:
why do we ever neen 1.15 version, since immediately after that we have 1.16 which completely rewrite what was done in previous one? This applies to the whole patchset: there are essentially several bug fixes and several features, so separate them into several patches, which do not collide with each other in the above way.
Signature · 1 line
Evgeniy Polyakov
Re: [PATCH 13/33 replaces 14/35 and 15/35] W1: w1_slave_read_id multiple short read bug
Quoted 11 lines · Evgeniy Polyakov
Hi David.
Quoted 4 lines · David Fries
This is more to point out the bug in
w1_slave_read_id, the next patch rewrites the routine.
w1.c1.15...
Quoted 1 line
w1.c1.16
Quoted 7 lines
That is what I talked about. I agree with your analysis of the problem and I agree with fixes. But I do not agree with how you created them:
why do we ever neen 1.15 version, since immediately after that we have 1.16 which completely rewrite what was done in previous one? This applies to the whole patchset: there are essentially several bug fixes and several features, so separate them into several patches, which do not collide with each other in the above way.
The old patch 14/35 and 15/35 was the only place I know of where I fixed the bug, then replaced it wholesale with something better. Several bugs, a few features, lots of patches, go figure.
Signature · 2 lines
David Fries <[email protected]> http://fries.net/~david/ (PGP encryption key available)
PATCH 34/35 Documentation/w1/masters/ds2490 update
In reply to David Fries===================================================================
This is an update to the previous set of patches, don't merge them yet. Some of the context was messed up and I don't expect patch to apply the updates with the previous set cleanly. I'll resubmit the entire set if these updates look good.
===================================================================
Documentation/w1/masters/ds2490 1.1 through 1.4 Provide some additional details about the status of the driver.
--- Documentation/w1/masters/ds2490 | 62 +++++++++++++++++++++++++++++++++++++++ 1 files changed, 62 insertions(+), 0 deletions(-)
Documentation/w1/masters/ds2490 +62 -0
@@ -16,3 +16,65 @@ which allows to build USB <-> W1 bridges. DS9490(R) is a USB <-> W1 bus master device which has 0x81 family ID integrated chip and DS2490 low-level operational chip. + +Notes and limitations. +- The weak pullup current is a minimum of 0.9mA and maximum of 6.0mA. +- The 5V strong pullup is supported with a minimum of 5.9mA and a + maximum of 30.4 mA. (From DS2490.pdf) +- While the ds2490 supports a hardware search the code doesn't take + advantage of it. +- The hardware will detect when devices are attached to the bus on the + next bus (reset?) operation, however only a message is printed as + the core w1 code doesn't make use of the information. Connecting + one device tends to give multiple new device notifications. +- The number of USB bus transactions could be reduced if w1_reset_send + was added to the API. The name is just a suggestion. It would take + a write buffer and a read buffer (along with sizes) as arguments. + It would add match rom and rom to the send buffer, reset the bus, + and read the result. The ds2490 block I/O command supports reset, + write, read, and strong pullup all in one command. That would + reduce the time and overhead for a set of commands. + + The core w1 functions required to do a temperature conversion are, + w1_reset_select_slave, w1_next_pullup, and w1_write_8, which turn + around and execute the master functions, reset_bus, write_block, + set_pullup, write_byte, set_pullup. Each ds2490 function will have + multiple USB bus transactions, except set_pullup where it only + requires transactions if the if the time value is different. The + conversion could be reduced to w1_next_pullup, w1_reset_send which + would call set_pullup, reset_send. The reset_send would enable reset + and strong pullup (if enabled), write the data buffer, execute the + block I/O command, read status, and get data. Currently I count 11 + transactions required, this would reduce it to 4. +- The hardware supports normal, flexable, and overdrive bus + communication speeds, but only the normal is supported. +- The registered w1_bus_master functions don't define error + conditions. If a bus search is in progress and the ds2490 is + removed it can produce a good amount of error output before the bus + search finishes. +- The hardware supports detecting some error conditions, such as + short, alarming presence on reset, and no presence on reset, but the + driver doesn't query those values. +- The ds2490 specification doesn't cover short bulk in reads in + detail, but my observation is if fewer bytes are requested than are + available, the bulk read will return an error and the hardware will + clear the entire bulk in buffer. It would be possible to read the + maximum buffer size to not run into this error condition, only extra + bytes in the buffer is a logic error in the driver. The code should + should match reads and writes as well as data sizes. Reads and + writes are serialized and the status verifies that the chip is idle + (and data is available) before the read is executed, so it should + not happen. +- Running x86_64 2.6.24 UHCI under qemu 0.9.0 under x86_64 2.6.22-rc6 + with a OHCI controller, ds2490 running in the guest would operate + normally the first time the module was loaded after qemu attached + the ds2490 hardware, but if the module was unloaded, then reloaded + most of the time one of the bulk out or in, and usually the bulk in + would fail. qemu sets a 50ms timeout and the bulk in would timeout + even when the status shows data available. A bulk out write would + show a successful completion, but the ds2490 status register would + show 0 bytes written. Detaching qemu from the ds2490 hardware and + reattaching would clear the problem. usbmon output in the guest and + host did not explain the problem. My guess is a bug in either qemu + or the host OS and more likely the host OS. +-- 03-06-2008 David Fries <[email protected]>
Signature · 1 line
1.4.4.4
Hi.
Quoted 8 lines · David Fries
I dug out a panic before I started modifying the driver, see the end of the message. There is still a remaining oops. Register the same sensor with two masters. The serial id of the sensor is assumed to be unique and the sysfs code will cause an oops when the second sensor is registered. If you have two master devices that are both doing bus searching, let one detect the sensor, the move it to the other master. The second master will detect the device before the first one times it out. None of my patches address this.
This is first time you show me the oops, so I we can start thinking about bug itself. Getting that you already have a fix and changes which I agree with, we can proceed with them. The only stuff I complain about it how you organized your patchset. There is no need to put multiple revisions of the same file in several patches when each new change just overwrite previous one, this complicates code review and digging into changeset changelogs in case of problems.
Quoted 5 lines
Could you be more specific on "really wrong, like what you call a deadlock was intended"? I only use 'intended' once, and that is is patch 1 in "The slave reconnect was incorrectly coded." That paragraph is describing a infinite loop inserting a slave driver, not unloading a device driver.
I had a gpio driver which worked really bad, since current source was very weak and shared with different resource. It was not pure luck to get data from it, but it did not work reliably. So there are all those loops you see in reading path, having them in userspace would provide a great feeling of really broken hardware (like that broked rom data you see blacklisted in the driver), while it just did not worked 100% reliably. Because of that I did not allow to forcedly stop 'transaction', since getting it right took lots of time, so we do want that data, so no interrupt, so removing module stops...
Yes, this is really ugly, but it works and was added intentionally. Now I think that for better quality and simplicity of the code we can drop this behaviour.
Quoted 17 lines
If something is really wrong with the descriptions I'll fix it.
Quoted 3 lines
There is another problem with your submission - please split it into 3-5 patches which are logically compound, so that there would not be things
like: patch1: replace A with B, patch2: remove B and the like.You must be talking about patch 14 and patch 15
w1_slave_read_idsort read bug. That's the only place it does that.I debated on how to submit
patch 14 "w1_slave_read_idmultiple short read bug"
and
patch 15 "w1_slave_read_idfrombin_attributetodevice_attribute" and I did it that way to first show what he bug was and fix it, then show how simple the code was using a different method. I'll submit it in one patch.
Quoted 2 lines
Quoted 1 line
You also have lots of codying style issues in the patchset.
Such as?
if( without space
a=b without spaces and the like. Pretty trivial, but I will be frowned upon if accept that :)
Quoted 2 lines
This was the overview e-mail, and it was long enough without going into details on each of the issues. But here they are.
I agree with what you pointed is not correct, including 'deadlock', but I already described why I did it that way. It does work and is ugly and is probably wrong, so I agreed to remove this 'feature'.
The same applies to other bug fixes you provided: I agreew ith them (even though some of them I see first time)
Quoted 10 lines
Quoted 9 lines
Please reorganize your patches into something like this:
1. threads changes
2. master device removal changes (remove slaves which are on the bus master being removed) and related things
3. ds2490 changes
4. cleanupsor something like that, reviewing 35 small patches which are removing things past another is not a really good time...
Quoted 7 lines
You want me to take 35 patches and merge them down to 4? There were a lot of issues in here, and I don't have an automagic patch reorder tool. If you want to review fewer patches apply 1-6, do a diff and look at that.
Documentation/SubmittingPatches'3) Separate your changes.' It goes on to specifically separate out bug fixes and performance enhancements.
Your changes overwrite each other, and some times adds something or remove something to accomplish requested goal. I specially pointed to one of them in your last patchset, which you marked rfc and not for merge. Change separation does not mean all commits should be sent as independent patch, logical changes should be combined. When patch 2 overwrite patch1 and does something similar, combine them. Bug fixes to bug fixes, features to features. If some overwrite fix a bug, send that overwrite.
Quoted 5 lines
You requested in another e-mail to merge patch 1-6. The deadlocks in 1 is clearly a bug. 6 block (instead of sleeping with a timeout) is a performance enhancement. 5 might be a deadlock, but it's not at all related to 1. 2 and 3 have nothing to do with any of the above. Logically this come out to be a bunch of patches.
I roughly pointed how to organize them. My main intention was to logically separate changes, not sending every single commit in different patch.
Quoted 5 lines · Andrew Morton
Quoted 4 lines · On Mon, 21 Jan 2008 20:47:19 -0600 David Fries <[email protected]> wrote:
Quoted 2 lines
Thanks. Question, at what point should I just submit these one liner, but independent bugs in one patch?
Quoted 8 lines
Quoted 1 line
You shouldn't, unless they are dependent in both directions.
If Andrew Morton is going to say no for independent one line bugs, I'm going to say No (for a bunch of complicated sometimes overlapping bugs, features, and performance enhancements) as well. Besides I have something like 27KB in 664 lines of descriptions, if I tried to merge the changes down there's no way I could figure out which patch lines went with what descriptions, and no one else could either.
I think I have to note that again: if some feature overwrite fix the bug, then just send that overwrite, since it was original goal. Instead you send number of one-line fixes, and then overwrite them. Less changes we have (presumbly they are logically separated and do not break each others), less time we will spend reviewing code, searching for wrong commit, checking for bugs.
I understand that you do not want to do that just because you already have cvs sommit log which you send to me, but this also means that I will need to do that for your changes and then resubmit.
Quoted 12 lines
w1_master_driverw1_bus_master1: Family 21 for 21.34c0000039c5.ba is not registered. Failed to read 1-wire data from0x02: err=-110.
Failed to read 1-wire data from0x02: err=-110.
usb 2-3: USB disconnect, address 5w1_master_driverw1_bus_master1: Waiting forw1_bus_master1to become free: refcnt=1.w1_slave_release: Releasing 81-00000020bbd0.w1_slave_release: Releasing 21-34c0000039c5.w1_slave_release: Releasing 28-0000000e84a2.w1_slave_release: Releasing 81-00000020bbd0.usb 2-3: new full speed USB device using
ohci_hcdand address 8 usb 2-3: device descriptor read/64, error -62
Quoted 8 lines
hub 2-0:1.0: Cannot enable port 3. Maybe the USB cable is bad? hub 2-0:1.0: Cannot enable port 3. Maybe the USB cable is bad? hub 2-0:1.0: Cannot enable port 3. Maybe the USB cable is bad? hub 2-0:1.0: Cannot enable port 3. Maybe the USB cable is bad? usb 2-3: new full speed USB device using ohci_hcd and address 12 usb 2-3: configuration #1 chosen from 1 choice usb 2-3: USB disconnect, address 12 BUG: unable to handle kernel NULL pointer dereference at virtual address 0000000c
Does this happen only when you remove and plug ds9490 back to the socket? We could fix it rather quickly, but I'm glad you fixed it yourself :)
Signature · 1 line
Evgeniy Polyakov