On Wed, Jul 22, 2020 at 07:04:00AM +0000, Johnson CH Chen (陳昭勳) wrote: > Hi Greg, > > Thanks for your response! > > > > > > + unsigned long flag; > > > > > + unsigned char cmd_buffer[84]; > > > > > + unsigned char rsp_buffer[84]; > > > > > > > > You seem to have two "static" buffers here, for your device, that > > > > you semi-randomly write to all over the place, but I can't find > > > > any locking or coordination between things that prevents multiple > > > > commands from not just overwritting each other. > > > > > > > For cmd_buffer[], we use npreal_wait_and_set_command() to make sure > > > cmd_buffer[] is safe to be written by checking "cmd_buffer[0] == 0". > > > > And what locks are protecting you there? > > > > > For rsp_buffer[], we use npreal_wait_command_completed() to make > > > sure rsp_buffer[] is desired by checking rsp_buffer[0] and rsp_buffer[1]. > > > Command_set and command should be checked. Besides, rsp_buffer[] is > > > got from user space by "NPREAL_NET_CMD_RESPONSE" in > > > npreal_net_ioctl(). > > > > Again, what locking is really handling this? > > > > It's better to protect cmd_buffer[84] and rsp_buffer[84] by locking completely. They are safe because NPort driver should be worked with NPort daemon before, and NPort daemon is designed to be simple. I'm sorry, but I do not understand this answer at all. Something can be "simple" and still be totally wrong :) Without locking, this code is broken. thanks, greg k-h
