| Message ID | 4931201D.7060701@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
roel kluin <roel.kluin@gmail.com> writes: > - if (--hvcsd->open_count == 0) { > + if (hvcsd->open_count == 1) { > + hvcsd->open_count--; This is not the same. > - if (--hp->count == 0) { > + if (hp->count == 1) { > + hp->count--; Likewise. Andreas.
Andreas Schwab wrote: > roel kluin <roel.kluin@gmail.com> writes: > >> - if (--hvcsd->open_count == 0) { >> + if (hvcsd->open_count == 1) { >> + hvcsd->open_count--; > > This is not the same. I think you're missing that I also decrement if (hvcsd->open_count > 1) If not, please elaborate. > >> - if (--hp->count == 0) { >> + if (hp->count == 1) { >> + hp->count--; > > Likewise. Same here. Roel
roel kluin <roel.kluin@gmail.com> writes: > Andreas Schwab wrote: >> roel kluin <roel.kluin@gmail.com> writes: >> >>> - if (--hvcsd->open_count == 0) { >>> + if (hvcsd->open_count == 1) { >>> + hvcsd->open_count--; >> >> This is not the same. > > I think you're missing that I also decrement if (hvcsd->open_count > 1) Yes, sorry, I missed that. Andreas.
roel kluin writes: > unsigned hp->count and hvcsd->open_count cannot be negative ... > - if (--hvcsd->open_count == 0) { > + if (hvcsd->open_count == 1) { > + hvcsd->open_count--; Why are we no longer decrementing hvcsd->open_count in the cases where it is greater than 1? > - if (--hp->count == 0) { > + if (hp->count == 1) { > + hp->count--; Ditto with hp->count here? Also, I don't see why those changes have anything to do with "unsigned things cannot be negative". As long as those counts are never zero on entry to those code sections, the existing code is fine, and I believe that assertion can be maintained. If you believe the code needs to defend against the possibility of a zero count on entry, that should have been explicitly stated in the patch description. Paul.
> Also, I don't see why those changes have anything to do with "unsigned > things cannot be negative". As long as those counts are never zero on > entry to those code sections, the existing code is fine, and I believe > that assertion can be maintained. If you believe the code needs to > defend against the possibility of a zero count on entry, that should > have been explicitly stated in the patch description. See the tty_port patches in -next - this is an argument about code which ought eventually to go away replaced by standard helper functions.
Alan Cox writes: > See the tty_port patches in -next - this is an argument about code which > ought eventually to go away replaced by standard helper functions. OK. In that case I think the best fix for the existing code is just to change the count to be int rather than unsigned int. Paul.
diff --git a/drivers/char/hvcs.c b/drivers/char/hvcs.c index 473d9b1..b228b84 100644 --- a/drivers/char/hvcs.c +++ b/drivers/char/hvcs.c @@ -1222,7 +1222,8 @@ static void hvcs_close(struct tty_struct *tty, struct file *filp) hvcsd = tty->driver_data; spin_lock_irqsave(&hvcsd->lock, flags); - if (--hvcsd->open_count == 0) { + if (hvcsd->open_count == 1) { + hvcsd->open_count--; vio_disable_interrupts(hvcsd->vdev); @@ -1248,7 +1249,9 @@ static void hvcs_close(struct tty_struct *tty, struct file *filp) free_irq(irq, hvcsd); kref_put(&hvcsd->kref, destroy_hvcs_struct); return; - } else if (hvcsd->open_count < 0) { + } else if (hvcsd->open_count > 1) { + hvcsd->open_count--; + } else { printk(KERN_ERR "HVCS: vty-server@%X open_count: %d" " is missmanaged.\n", hvcsd->vdev->unit_address, hvcsd->open_count); diff --git a/drivers/char/hvsi.c b/drivers/char/hvsi.c index 59c6f9a..d46bccd 100644 --- a/drivers/char/hvsi.c +++ b/drivers/char/hvsi.c @@ -875,7 +875,8 @@ static void hvsi_close(struct tty_struct *tty, struct file *filp) spin_lock_irqsave(&hp->lock, flags); - if (--hp->count == 0) { + if (hp->count == 1) { + hp->count--; hp->tty = NULL; hp->inbuf_end = hp->inbuf; /* discard remaining partial packets */ @@ -908,7 +909,9 @@ static void hvsi_close(struct tty_struct *tty, struct file *filp) spin_lock_irqsave(&hp->lock, flags); } - } else if (hp->count < 0) + } else if (hp->count > 1) + hp->count--; + else printk(KERN_ERR "hvsi_close %lu: oops, count is %d\n", hp - hvsi_ports, hp->count);
unsigned hp->count and hvcsd->open_count cannot be negative Signed-off-by: Roel Kluin <roel.kluin@gmail.com> --- Both members of respectively struct hvcs_struct, see vi drivers/char/hvcs.c +262 and struct hvcs_struct, see vi drivers/char/hvsi.c +70